Re: [Pki-devel] [PATCH] 0084..0086 Lightweight CA replication support

A few comments. 1. One of the first things that struck me as odd was making CertificateAuthority implement Runnable. I think it would be cleaner to have a static inner class called AuthorityMonitor or similar to which we pass in the CertificateAuthority. 2. I do like the fact that the caMap updates are done through a static database connection factory created by the hostCA. How do you ensure that the database connection factory is created before being used by other CAs? 3. I'm not sure I understand how the initialLoadDone counter is supposed to work. Are all the CA's supposed to stop until the hostCA has completed its initial load? Because it looks like only the hostCA calls await(). 4. There is a lot of code in that initial patch. It would help review to split that off into at least two patches, say one in which you add the functions in CertificateAuthority that handle modifications in the caMap based on persistent search results, and one which adds the new monitor thread. 5. Some in-code documentation would not go amiss. For instance, I have no idea why this code is correct -- String[] objectClasses = entry.getAttribute("objectClass").getStringValueArray(); if (Arrays.asList(objectClasses).contains("organizationalUnit")) { initialNumAuthorities = new Integer( entry.getAttribute("numSubordinates") .getStringValueArray()[0]); checkInitialLoadDone(); continue; } organizationalUnit? There are lots of different variables like initialNumAuthorities etc. which could potentially be hidden in an inner class, making this more understandable. Ade On Tue, 2016-03-22 at 16:00 +1000, Fraser Tweedale wrote: