[Date Prev][Date Next] [Chronological] [Thread] [Top]

Re: (ITS#7182) back-ldap monitoring improvements

Ondrej Kuznik wrote:
Hash: SHA1

On 03/27/2012 11:25 AM, Howard Chu wrote:
Your patch 4 seems to be quite invasive already. How will the new monitor
output compare to the existing output? Will this break any scripts that were
using the old monitor entries?

Thanks for the input, when comparing the data before submission briefly,
I missed the difference that olmDbURIList and labeledURI are in
"cn=Connections,cn=database #,..." in HEAD while after my patching they
ended up in the "cn=database #,..." monitor entry (where they make more
sense but could indeed make for an incompatible change). I will update
this in the next patchset. There should be no other compatibility issues
as there was no other useful information and slapo-chain seems not to
actually enable monitoring of its backends (fixing that would make for
another change I do not have ready).

OK. I'm not too concerned with slapo-chain, we can note that as a future enhancement as well.

You seem to have no-op'd out the monitor_db_close() cleanups. It would be a
good idea to do all of the proper unregister/cleanup here.

Since I dropped the ad-hoc entry creation, there are now only the
monitor subsystems registered, for which there currently is no
unregister callback. During slapd shutdown back-monitor handles the
destruction and callbacks itself, the only time the no-oping could hurt
is when SLAPD_CONFIG_DELETE is enabled (as mentioned in the comment).

I can certainly add that callback to back-monitor bi->extra and use it,
however such change would either be a little naive or require changing
the actual type of the monitor_subsys variable to an AVL or an
LDAP_LIST_*. I did not feel confident mucking with this part of
back-monitor myself and judged that confining the intrusive changes only
to back-ldap/monitor.c would make the patchset's chances of inclusion
somewhat higher.

As you are the person to make the call, what is your opinion on this?
Shall I update the patch to do proper subsystem deregistration (and
prepare another that introduces such a thing to back-monitor), leave
that part as it is or do something completely different?

OK, it sounds fine to ignore this for now.

  -- Howard Chu
  CTO, Symas Corp.           http://www.symas.com
  Director, Highland Sun     http://highlandsun.com/hyc/
  Chief Architect, OpenLDAP  http://www.openldap.org/project/