10:56 p.m.
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
1:20 p.m.
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();
}
}
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);
}
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).
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);
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.
More will follow.
--
Endi S. Dewata
6:07 p.m.
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
8:07 p.m.
Rebase was needed for the remaining patches; 0060-2 and 0061-2
attached.
Thanks,
Fraser
12:44 p.m.
On 1/19/2016 8:07 PM, Fraser Tweedale wrote:
Rebase was needed for the remaining patches; 0060-2 and 0061-2
attached.
Thanks,
Fraser
ACK for both of them.
Just wondering, would it be possible to distinguish local and remote
persistent search results? So if a profile is changed on a machine, the
profile object will be updated immediately in memory, then persistent
search result for that modification will be ignored since it's initiated
locally. Other machines, however, will pick up the changes from the
persistent search result since it's initiated remotely.
--
Endi S. Dewata
6:06 p.m.
On Wed, Jan 20, 2016 at 12:44:38PM -0600, Endi Sukma Dewata wrote:
On 1/19/2016 8:07 PM, Fraser Tweedale wrote:
>Rebase was needed for the remaining patches; 0060-2 and 0061-2
>attached.
>
>Thanks,
>Fraser
>
ACK for both of them.
Thanks! Pushed to master
- 41717cb774f53e30caf7a57c2e07526445bf0988 Ensure config store commits refresh file-based
profile data
- 0af4a9393ce38216689e9afe17514b9441784133 Handle LDAPProfileSubsystem
delete-then-recreate races
Just wondering, would it be possible to distinguish local and remote
persistent search results? So if a profile is changed on a machine, the
profile object will be updated immediately in memory, then persistent search
result for that modification will be ignored since it's initiated locally.
Other machines, however, will pick up the changes from the persistent search
result since it's initiated remotely.
This is indeed accomplished by the tracking of the entryUSN
resulting from a change (via the PostReadControl) and the deleted
entries.
Cheers,
Fraser
7:55 p.m.
On Thu, Jan 21, 2016 at 10:06:41AM +1000, Fraser Tweedale wrote:
On Wed, Jan 20, 2016 at 12:44:38PM -0600, Endi Sukma Dewata wrote:
> On 1/19/2016 8:07 PM, Fraser Tweedale wrote:
> >Rebase was needed for the remaining patches; 0060-2 and 0061-2
> >attached.
> >
> >Thanks,
> >Fraser
> >
>
> ACK for both of them.
>
Thanks! Pushed to master
- 41717cb774f53e30caf7a57c2e07526445bf0988 Ensure config store commits refresh file-based
profile data
- 0af4a9393ce38216689e9afe17514b9441784133 Handle LDAPProfileSubsystem
delete-then-recreate races
All five commits have been pushed to DOGTAG_10_2_BRANCH:
* 1c9cbd1 Ensure config store commits refresh file-based profile data
* 7957c80 Handle LDAPProfileSubsystem delete-then-recreate races
* 2cb2e9c Avoid profile race conditions by tracking entryUSN
* 6371ea5 Add LDAPPostReadControl class
* 6ed6230 Extract LDAPControl search function to LDAPUtil
8:52 a.m.
On 1/20/2016 6:06 PM, Fraser Tweedale wrote:
On Wed, Jan 20, 2016 at 12:44:38PM -0600, Endi Sukma Dewata wrote:
> On 1/19/2016 8:07 PM, Fraser Tweedale wrote:
>> Rebase was needed for the remaining patches; 0060-2 and 0061-2
>> attached.
>>
>> Thanks,
>> Fraser
>>
>
> ACK for both of them.
>
Thanks! Pushed to master
- 41717cb774f53e30caf7a57c2e07526445bf0988 Ensure config store commits refresh file-based
profile data
- 0af4a9393ce38216689e9afe17514b9441784133 Handle LDAPProfileSubsystem
delete-then-recreate races
> Just wondering, would it be possible to distinguish local and remote
> persistent search results? So if a profile is changed on a machine, the
> profile object will be updated immediately in memory, then persistent search
> result for that modification will be ignored since it's initiated locally.
> Other machines, however, will pick up the changes from the persistent search
> result since it's initiated remotely.
>
This is indeed accomplished by the tracking of the entryUSN
resulting from a change (via the PostReadControl) and the deleted
entries.
Right. I was just thinking suppose the persistent search result contains
a 'modifiedBy' attribute that stores the hostname, we might be able use
that to ignore local changes without storing anything in the memory.
--
Endi S. Dewata
3011
days inactive
3062
days old
7 comments
2 participants
participants (2)
-
Endi Sukma Dewata -
Fraser Tweedale