Re: [Pki-devel] [PATCH] pki-ftweedal-0015-Monitor-database-for-changes-to-LDAP-profiles.patch

On 10/1/2014 1:42 AM, Fraser Tweedale wrote: >On Mon, Sep 22, 2014 at 05:36:26PM +1000, Fraser Tweedale wrote: >>This is the first cut of the LDAP profile change monitoring. It >>depends on patches 0004..0009 and 0014 >>(https://www.redhat.com/archives/pki-devel/2014-September/msg00052.html). >> >>To summarise the implementation: a separate thread carries out a >>persistent LDAP search and calls back into the ProfileSubsystem when >>changes occur. I haven't had much experience with persistent >>searches or multithreaded programming in Java, so eyeballs familiar >>with those areas are needed. >> >>I haven't yet tested with changes replicating between clones (a task >>for tomorrow) but I wanted to get the patch on list for feedback as >>early as possible. >> >Further to earlier email, I tested profile change replication >between clones and it is working well. > Some comments: 1. The MODDN operation can potentially happen if someone renames the profile in LDAP directly, and that's easier to do with LDAP-based profiles. I think we should handle this case too. 2. If the LDAP server is restarted the Monitor will stop working and subsequent changes are not detected. The Monitor should automatically reconnect in case the LDAP connection is interrupted. 3. Since the changesOnly is set to true, there's a possibility that changes that happens after initial profile loading and before the Monitor starts will not be detected. To avoid this problem, the changesOnly should be false, and the initial profile loading should be done in the Monitor as well. 4. The following assignment is redundant because the value is overwritten by the next line: String profileId = "<unknown>"; 5. When receiving a MODIFY operation the Monitor calls forgetProfile(). It's probably redundant because readProfile() calls createProfile() which calls forgetProfile() too. 6. Generally it's recommended to implement Runnable instead of extending Thread. The Monitor can be merged into ProfileSubsystem. I think #1, #2, #3 are important to fix because there is no mechanism to refresh the profiles in memory other than by restarting the server, so we cannot miss any changes in the database. -- Endi S. Dewata