Alon Bar-Lev has posted comments on this change. Change subject: aaa: Fix sync ......................................................................
Patch Set 13: (6 comments) http://gerrit.ovirt.org/#/c/28561/13/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/sync/SyncAAAEntries.java File backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/sync/SyncAAAEntries.java: Line 30: // 3. Fetch groups from DB Line 31: // 4. Resolve the fetched groups (for each group that was not already resolved) Line 32: // 5. Resolve recursively all groups Line 33: // 6. Compare the user from db and the fetched user from the directory + store in DB if there is a change Line 34: public void sync( > well, this is not just fetching user, but information that can be used for same same. a list of PrincipalRecord resolved recursively with GroupRecords, are the data structure you need in order to sync. the actual fetch algorithm is totally detached from sync. Line 35: Map<String, Map<String, Set<String>>> userIdsToFetch, Line 36: Set<String> activeGroupIds, Line 37: Map<String, DirectoryGroup> outDirectoryGroupsByIds, Line 38: Map<String, DirectoryUser> outDirectoryUserByIds, Line 31: // 4. Resolve the fetched groups (for each group that was not already resolved) Line 32: // 5. Resolve recursively all groups Line 33: // 6. Compare the user from db and the fetched user from the directory + store in DB if there is a change Line 34: public void sync( Line 35: Map<String, Map<String, Set<String>>> userIdsToFetch, > hm, i thought about it, but u have 3 dimensions here - as I wrote, the fetchPrincipals logic can be per authz, there is no reason to do this cross authz as there is nothing common. Line 36: Set<String> activeGroupIds, Line 37: Map<String, DirectoryGroup> outDirectoryGroupsByIds, Line 38: Map<String, DirectoryUser> outDirectoryUserByIds, Line 39: List<String> outNonActiveUserIds, Line 103: } Line 104: groupIdsToFetch = getIdsFromGroups(groupsToFetch, fetchedGroupsCache); Line 105: } Line 106: } Line 107: outDirectoryGroupsByIds.putAll(fetchedGroupsCache); > what do you mean ? the outXXX means this is an "out" parameter, wheres fetc why do you need both outDirectoryGroupsByIds and fetchedGroupsCache? anyway, when you fix the signature all will be removed... List<PrincipalRecord> fetchPrincipals(Authz, List<PrincipalId>); Line 108: } Line 109: Line 110: Line 111: private void addGroupsToFetch(Set<String> activeGroupIds, http://gerrit.ovirt.org/#/c/28561/13/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/aaa/DirectoryUtils.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/aaa/DirectoryUtils.java: Line 59: dbGroupsPerExternalId.put(dbGroup.getExternalId(), dbGroup); Line 60: if (dbGroup.isActive()) { Line 61: activeGroupExternalIds.add(dbGroup.getExternalId()); Line 62: } Line 63: } > we're constructing activeGroupExternalIds structure to know which groups ar but if we start from users, we never get inactive group. Line 64: Line 65: Map<String, DirectoryGroup> outDirectoryGroupsByIds = new HashMap<>(); Line 66: Map<String, DirectoryUser> outDirectoryUserByIds = new HashMap<>(); Line 67: List<String> outNonActiveUserExternalIds = new ArrayList<>(); Line 80: Line 81: for (String id : outNonActiveGroupExternalIds) { Line 82: dbGroupsPerExternalId.get(id).setActive(false); Line 83: DbFacade.getInstance().getDbGroupDao().update(dbGroupsPerExternalId.get(id)); Line 84: } > what do you mean here? if you have outDirectoryUsers and you have a collection of all database users, you can conclude what are missing from the list (inactive) without have outNonActiveXXXX parameter. Line 85: Line 86: for (DbUser dbUser : dbUsers) { Line 87: if (dbUser.isActive()) { Line 88: DirectoryUser user = outDirectoryUserByIds.get(dbUser.getExternalId()); Line 86: for (DbUser dbUser : dbUsers) { Line 87: if (dbUser.isActive()) { Line 88: DirectoryUser user = outDirectoryUserByIds.get(dbUser.getExternalId()); Line 89: Set<DirectoryGroup> accumulator = new HashSet<>(); Line 90: DirectoryUtils.flatGroups(accumulator, user.getGroups(), outDirectoryGroupsByIds); > I think the flat should not be here and should exist at the aaa sync. once again... you are mixing the layers. the fact that the bll code stores the group per user in flat way should not leak into this implementation. this implementation should be pure, in the sense of return valid data structure that can be used for any purpose. a valid pure data structure is PrincipalRecord whose GroupRecords is resolved. we do not do this at extension level as we can optimize it. the fact that we optimize it, does not mean we should invent new data structures. so let's start again... if no optimization should have been done we would have the following algorithm: List principals; for (principalid : principalsids) { principals.add(authz.fetchPrincipal(principalid)); } return principals; please confirm, that the above would have been sufficient but not optimized, as we would have executed N queries per N principals and N*M for groups. then we have added one level of optimization: List principals; principals.add(authz.queryPrincipals(query: principalids = principalids), recursive=true); return principals; this optimization would query N/max_per query and N*M for groups, but will return same data structure. now... we want to avoid fetching common groups, so we add algorithm with same signature (return a list of principals) but avoid using recursive. so, as you can see the optimization considerations NEVER change the interface, only the implementation, it is not leaking out of logic. also the database consideration should not leak into the logic of fetch. please try to have clear cut between components. aaa - use only PrincipalRecord/GroupRecord and primitive types, no database considerations are allowed. bll - use database entities, map between PrincipalRecord/GroupRecord to DirectoryXXXX, while mapping add whatever database considerations you want. Line 91: user.setGroups(new ArrayList<DirectoryGroup>(accumulator)); Line 92: DbUser userFromDirectory = new DbUser(user); Line 93: userFromDirectory.setGroupIds(DirectoryUtils.getGroupIdsFromUser(user, outDirectoryGroupsByIds)); Line 94: if (!userFromDirectory.equals(dbUser)) { -- 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: 13 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
