Hi Endi, thanks for reviewing. I've addressed your comments (commentary inline) and pushed 57..59 to master:
0057 d272cec2614a4a45abd3fdbf7139dbd52b3275ae 0058 2bd89f148b4b347fc80285ec521d2af0299da746 0059 81af68d3e3b1a89f799693e7f7ecda59f57abfe4 On Tue, Jan 12, 2016 at 01:20:47PM -0600, Endi Sukma Dewata wrote: > On 11/30/2015 10:56 PM, Fraser Tweedale wrote: > >The attached patches resolve concurrency issues in the > >LDAPProfileSubsystem (which conducts its LDAP persisent search in > >background thread). > > > >Ticket: https://fedorahosted.org/pki/ticket/1700 > > > >Thanks, > >Fraser > > Patch #57 is ACKed. > > Patch #58 is ACKed with some comments: > > 1. To help troubleshooting, the static initializer in LDAPPostReadControl > should log the exception: > > static { > try { > register(OID_POSTREAD, LDAPPostReadControl.class); > } catch (Throwable e) { > e.printStackTrace(); > } > } > Done. > 2. In LDAPPostReadControl constructor is it necessary to catch & ignore the > exception? I think the String constructor doesn't declare any throwable > exceptions. If necessary, we could wrap the original exception and rethrow > it so we'll know if there's an error. > > try { > dn = new String(buf, "UTF8"); > } catch(Throwable e) { > throw IOException(e); > } > This constructor can throw UnsupportedEncodingException which is a subclass of IOException. IOException is declared by the method so I changed it to just let the exception through. > 3. The LDAPPostReadControl is used for both request and response. It's not > an issue, but it would be nice to use separate classes since they have > different contents (e.g. the request doesn't have an LDAP entry). > Not possible, unfortunately. The same OID is used for both the request and response structures, and the control is registered by OID. > Patch #59 is ACKed with some comments: > > 1. In AbstractProfileSubsystem and LDAPProfileSubsystem, the commitProfile() > should chain the original exception to help troubleshooting: > > throw new EProfileException( > "Failed to commit config store of profile '" + id + ": " + e, > e); > Done, thanks. > 2. In LDAPProfileSubsystem it would be nice to have a time- or size-based > cache expiration for the entryUSNs map, but assuming the number of profiles > will stay relatively small this may not be a problem. Just something to keep > in mind. > Size is linear in number of profiles; nothing to worry about :) > More will follow. > > -- > Endi S. Dewata > Cheers, Fraser _______________________________________________ Pki-devel mailing list [email protected] https://www.redhat.com/mailman/listinfo/pki-devel
