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