You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ambari.apache.org by Oliver Szabo <os...@hortonworks.com> on 2016/02/11 14:39:58 UTC
Review Request 43482: Ldap Sync: Concurrent modification exception
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43482/
-----------------------------------------------------------
Review request for Ambari, Robert Levas and Robert Nettleton.
Bugs: AMBARI-15013
https://issues.apache.org/jira/browse/AMBARI-15013
Repository: ambari
Description
-------
Concurrent modification exception can occour during ldap sync. (not always)
Diffs
-----
ambari-server/src/main/java/org/apache/ambari/server/security/ldap/AmbariLdapDataPopulator.java 21492cf
ambari-server/src/test/java/org/apache/ambari/server/security/ldap/AmbariLdapDataPopulatorTest.java be92871
Diff: https://reviews.apache.org/r/43482/diff/
Testing
-------
Unit testiong is in progress
Thanks,
Oliver Szabo
Re: Review Request 43482: Ldap Sync: Concurrent modification exception
Posted by Oliver Szabo <os...@hortonworks.com>.
> On Feb. 11, 2016, 2:08 p.m., Robert Nettleton wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/security/ldap/AmbariLdapDataPopulator.java, line 298
> > <https://reviews.apache.org/r/43482/diff/1/?file=1240255#file1240255line298>
> >
> > Could we get some more background on the root of the original exception?
> >
> > The change in this patch seems straightforward, in that it looks like there must be duplicate entries for a given group in the "internalGroupsMap".
> >
> > Is having duplicate groups with the same name a valid condition?
> >
> > From a quick glance, it looks like this class obtains the list of internal groups from:
> >
> > org.apache.ambari.server.security.ldap.AmbariLdapDataPopulator#getInternalGroups
> >
> > which appears to query the DB in order to obtain this information.
> >
> > Could this problem be caused by an invalid database state?
> >
> > I'm not against this change as it is, but I do think we should know more about why the DB includes duplicate groups, and if that is a valid condition.
> >
> > Thanks.
It's only java related issue, with nested groups, we remove a group from internalGroupMap (method: addLdapGroup), and then we try to use the reference later during the iteration. It causes ConcurrentModificationException, because ".values()" are act as view of the values in the Map, so now I just copy the values into a Set, and iterate over them (in the iteration the name only used).
Long term, sync should be rewritten, especially "refreshGroupMembers" method, we should try to avoid using recursion like that
- Oliver
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43482/#review118868
-----------------------------------------------------------
On Feb. 11, 2016, 1:39 p.m., Oliver Szabo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43482/
> -----------------------------------------------------------
>
> (Updated Feb. 11, 2016, 1:39 p.m.)
>
>
> Review request for Ambari, Robert Levas and Robert Nettleton.
>
>
> Bugs: AMBARI-15013
> https://issues.apache.org/jira/browse/AMBARI-15013
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Concurrent modification exception can occour during ldap sync. (not always)
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/security/ldap/AmbariLdapDataPopulator.java 21492cf
> ambari-server/src/test/java/org/apache/ambari/server/security/ldap/AmbariLdapDataPopulatorTest.java be92871
>
> Diff: https://reviews.apache.org/r/43482/diff/
>
>
> Testing
> -------
>
> Unit testiong is in progress
>
>
> Thanks,
>
> Oliver Szabo
>
>
Re: Review Request 43482: Ldap Sync: Concurrent modification exception
Posted by Robert Nettleton <rn...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43482/#review118868
-----------------------------------------------------------
Fix it, then Ship it!
The patch looks fine, but I think this needs more investigation, to determine why duplicate groups are included in the DB.
Thanks.
ambari-server/src/main/java/org/apache/ambari/server/security/ldap/AmbariLdapDataPopulator.java (line 298)
<https://reviews.apache.org/r/43482/#comment180181>
Could we get some more background on the root of the original exception?
The change in this patch seems straightforward, in that it looks like there must be duplicate entries for a given group in the "internalGroupsMap".
Is having duplicate groups with the same name a valid condition?
From a quick glance, it looks like this class obtains the list of internal groups from:
org.apache.ambari.server.security.ldap.AmbariLdapDataPopulator#getInternalGroups
which appears to query the DB in order to obtain this information.
Could this problem be caused by an invalid database state?
I'm not against this change as it is, but I do think we should know more about why the DB includes duplicate groups, and if that is a valid condition.
Thanks.
- Robert Nettleton
On Feb. 11, 2016, 1:39 p.m., Oliver Szabo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43482/
> -----------------------------------------------------------
>
> (Updated Feb. 11, 2016, 1:39 p.m.)
>
>
> Review request for Ambari, Robert Levas and Robert Nettleton.
>
>
> Bugs: AMBARI-15013
> https://issues.apache.org/jira/browse/AMBARI-15013
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Concurrent modification exception can occour during ldap sync. (not always)
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/security/ldap/AmbariLdapDataPopulator.java 21492cf
> ambari-server/src/test/java/org/apache/ambari/server/security/ldap/AmbariLdapDataPopulatorTest.java be92871
>
> Diff: https://reviews.apache.org/r/43482/diff/
>
>
> Testing
> -------
>
> Unit testiong is in progress
>
>
> Thanks,
>
> Oliver Szabo
>
>
Re: Review Request 43482: Ldap Sync: Concurrent modification exception
Posted by Robert Levas <rl...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43482/#review119310
-----------------------------------------------------------
Ship it!
Ship It!
- Robert Levas
On Feb. 11, 2016, 8:39 a.m., Oliver Szabo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43482/
> -----------------------------------------------------------
>
> (Updated Feb. 11, 2016, 8:39 a.m.)
>
>
> Review request for Ambari, Robert Levas and Robert Nettleton.
>
>
> Bugs: AMBARI-15013
> https://issues.apache.org/jira/browse/AMBARI-15013
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Concurrent modification exception can occour during ldap sync. (not always)
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/security/ldap/AmbariLdapDataPopulator.java 21492cf
> ambari-server/src/test/java/org/apache/ambari/server/security/ldap/AmbariLdapDataPopulatorTest.java be92871
>
> Diff: https://reviews.apache.org/r/43482/diff/
>
>
> Testing
> -------
>
> Unit testiong is in progress
>
>
> Thanks,
>
> Oliver Szabo
>
>