Alon Bar-Lev has posted comments on this change. Change subject: aaa: Fix sync ......................................................................
Patch Set 12: (16 comments) OK... I am still confused... And now I know why... the sync algorithm should work with low level authz entities, I do not see any need to mix the db entities and the authz entities. Logic should be: 1. import db entities into lists/hashes as authz ids. 2. perform the recursive build algoirhtm, output is set of users/groups as authz maps 3. translate the authz maps into db entities The entire logic at (2) should deal with single entity without any conversion. What do you think? http://gerrit.ovirt.org/#/c/28561/12/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/DbUserCacheManager.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/DbUserCacheManager.java: Line 49: // 2. Resolve the groups of users (get 1st level groups) Line 50: // 3. Fetch groups from DB Line 51: // 4. Resolve the fetched groups (for each group that was not already resolved) Line 52: // 5. Resolve recursively all groups Line 53: // 6. Compare the user from db and the fetched user from the directory + store in DB if there is a change comment should go with code? Line 54: @OnTimerMethodAnnotation("refreshAllUsers") Line 55: public void refreshAllUsers() { Line 56: sync.refreshAllUsers(DbFacade.getInstance().getDbUserDao().getAll()); Line 57: } http://gerrit.ovirt.org/#/c/28561/12/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/SyncAAAEntries.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/SyncAAAEntries.java: can we move this to aaa? or part of it? a part that is disconnected from db stuff? gets the list of ids and returns the groups. Line 1: package org.ovirt.engine.core.bll; Line 2: Line 3: import java.util.ArrayList; Line 4: import java.util.Arrays; Line 29: public class SyncAAAEntries { Line 30: Line 31: private static final Log log = LogFactory.getLog(SyncAAAEntries.class); Line 32: Line 33: public void refreshAllUsers(List<DbUser> dbUsers) { s/All//? Line 34: final Map<String, List<DbUser>> usersPerAuthz = new HashMap<>(); Line 35: final Map<String, List<DbGroup>> groupsPerAuthz = new HashMap<>(); Line 36: Line 37: for (DbUser dbUser : dbUsers) { Line 33: public void refreshAllUsers(List<DbUser> dbUsers) { Line 34: final Map<String, List<DbUser>> usersPerAuthz = new HashMap<>(); Line 35: final Map<String, List<DbGroup>> groupsPerAuthz = new HashMap<>(); Line 36: Line 37: for (DbUser dbUser : dbUsers) { why not do the isActive magic here? Line 38: MultiValueMapUtils.addToMap(dbUser.getDomain(), dbUser, usersPerAuthz); Line 39: } Line 40: Line 41: for (DbGroup dbGroup : DbFacade.getInstance().getDbGroupDao().getAll()) { Line 37: for (DbUser dbUser : dbUsers) { Line 38: MultiValueMapUtils.addToMap(dbUser.getDomain(), dbUser, usersPerAuthz); Line 39: } Line 40: Line 41: for (DbGroup dbGroup : DbFacade.getInstance().getDbGroupDao().getAll()) { this should not run when we sync addUser, right? Line 42: MultiValueMapUtils.addToMap(dbGroup.getDomain(), dbGroup, groupsPerAuthz); Line 43: } Line 44: Line 45: Set<String> authzNames = new HashSet<String>() { Line 38: MultiValueMapUtils.addToMap(dbUser.getDomain(), dbUser, usersPerAuthz); Line 39: } Line 40: Line 41: for (DbGroup dbGroup : DbFacade.getInstance().getDbGroupDao().getAll()) { Line 42: MultiValueMapUtils.addToMap(dbGroup.getDomain(), dbGroup, groupsPerAuthz); also here add the isActive magic Line 43: } Line 44: Line 45: Set<String> authzNames = new HashSet<String>() { Line 46: { Line 53: for (String authzName : authzNames) { Line 54: ExtensionProxy extension = null; Line 55: try { Line 56: extension = EngineExtensionsManager.getInstance().getExtensionByName(authzName); Line 57: } catch (Exception ex) { catch specific exception? Line 58: log.warn(String.format("The extension %1$s could not be found. Users associated with this authz extension will not be refreshed.", Line 59: authzName)); Line 60: } Line 61: refreshedUsers.addAll(refreshAllUsers(extension, Line 59: authzName)); Line 60: } Line 61: refreshedUsers.addAll(refreshAllUsers(extension, Line 62: usersPerAuthz.get(authzName), Line 63: groupsPerAuthz.get(authzName))); shouldn't this be within the try? Line 64: } Line 65: Line 66: for (DbUser dbUser : refreshedUsers) { Line 67: DbFacade.getInstance().getDbUserDao().update(dbUser); Line 62: usersPerAuthz.get(authzName), Line 63: groupsPerAuthz.get(authzName))); Line 64: } Line 65: Line 66: for (DbUser dbUser : refreshedUsers) { this refreshed term is confusing as these are to be refresh not already refreshed. Line 67: DbFacade.getInstance().getDbUserDao().update(dbUser); Line 68: } Line 69: } Line 70: Line 81: Map<String, List<String>> usersToFetchPerNamesace = new HashMap<>(); Line 82: Set<String> activeGroupIds = new HashSet<>(); Line 83: Line 84: for (DbUser dbUser : dbUsers) { Line 85: if (dbUser.isActive()) { this is way too late for isActive() Line 86: MultiValueMapUtils.addToMap(dbUser.getNamespace(), dbUser.getExternalId(), usersToFetchPerNamesace); Line 87: } Line 88: } Line 89: Line 87: } Line 88: } Line 89: Line 90: for (DbGroup dbGroup : dbGroups) { Line 91: if (dbGroup.isActive()) { way too late for isActive Line 92: activeGroupIds.add(dbGroup.getExternalId()); Line 93: } Line 94: } Line 95: boolean supportRecursiveGroupResolution = Line 104: true)) { Line 105: directoryUsersByIds.put(directoryUser.getId(), directoryUser); Line 106: Line 107: if (supportRecursiveGroupResolution) { Line 108: putGroupsInCache(cache, directoryUser.getGroups()); why do you mix the db groups at this point? you can cache the plain primitive results from authz, only after you have all the data, to transform it to db groups this entire algorithm should have input that is primitive, output that is primitive and a wrapper that translate the complex entities to and forth. I think this is the major complexity that exists in this implementation. Line 109: } else { Line 110: addGroupsToFetch(activeGroupIds, cache, groupsToFetch, directoryUser.getGroups()); Line 111: } Line 112: } Line 118: if (!directoryUsersByIds.containsKey(dbUser.getExternalId())) { Line 119: dbUser.setActive(false); Line 120: DbFacade.getInstance().getDbUserDao().save(dbUser); Line 121: } Line 122: } please update the database only after sync algorithm finished and you have all data in memory. Line 123: Line 124: for (DbGroup dbGroup : dbGroups) { Line 125: MultiValueMapUtils.addToMapOfSets(dbGroup.getNamespace(), dbGroup.getExternalId(), groupIdsToFetch); Line 126: } Line 138: cache.put(group.getId(), group); Line 139: addGroupsToFetch(activeGroupIds, cache, groupsToFetch, group.getGroups()); Line 140: } Line 141: } Line 142: groupIdsToFetch = getIdsFromGroups(groupsToFetch, cache); if you override this variable, you do not need to remove the namespace above... Line 143: } Line 144: List<DbUser> refreshed = new ArrayList<>(); Line 145: for (DbUser dbUser : dbUsers) { Line 146: DirectoryUser directoryUser = directoryUsersByIds.get(dbUser.getExternalId()); Line 247: // the flag that indicates that the database needs to be updated: Line 248: if (!StringUtils.equals(dbUser.getFirstName(), directoryUser.getFirstName())) { Line 249: dbUser.setFirstName(directoryUser.getFirstName()); Line 250: update = true; Line 251: } strange... what is the cost to just update all if anything (equals/hash) changed? Line 252: if (!StringUtils.equals(dbUser.getLastName(), directoryUser.getLastName())) { Line 253: dbUser.setLastName(directoryUser.getLastName()); Line 254: update = true; Line 255: } http://gerrit.ovirt.org/#/c/28561/12/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/DbGroup.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/DbGroup.java: Line 40: setId(new Guid(directoryGroup.getId().getBytes(), true)); Line 41: namespace = directoryGroup.getNamespace(); Line 42: domain = directoryGroup.getDirectoryName(); Line 43: name = directoryGroup.getName(); Line 44: active = true; so it is supported? but you wrote me that schema should be modified... Line 45: } Line 46: Line 47: public Guid getId() { Line 48: return id; -- To view, visit http://gerrit.ovirt.org/28561 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id49b51517a967c7a83e8e73f52181673baa31700 Gerrit-PatchSet: 12 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yair Zaslavsky <yzasl...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
