Juan Hernandez has posted comments on this change.
Change subject: [WIP] Introduce new directory interface
......................................................................
Patch Set 1: (4 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/directory/DirectoryManager.java
Line 55: /**
Line 56: * Loads the directory implementations that are available in the
class path
Line 57: * using the service loader mechanism.
Line 58: */
Line 59: private void loadImplementations() {
No, you don't want all jars in the classpath per default, as they may bring
many dependencies that you don't want. Or do you want the jar files that
implement the Atlassian crowd directory by default in the classpath? And the
keystore directory by default? Those should be in different .jar files,
supplied by different packages, and installed only if required.
The fact that you have a implementation in the classpath means that it will be
available, but it doesn't mean that it will provide any directory instance,
unless you configure it.
Line 60: ServiceLoader<DirectorySpi> loader =
Line 61: ServiceLoader.load(DirectorySpi.class);
Line 62: for (DirectorySpi implementation : loader) {
Line 63: implementations.add(implementation);
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/users/Directory.java
Line 50: * with no particular order, note that the returned set may
contain less
Line 51: * elements than the given set of identifiers and that it may be
in a
Line 52: * different order
Line 53: */
Line 54: Set<DirectoryUser> findUsersById(Set<Guid> ids);
Like I said, the UUIDs enable any implementation, as it is the responsibility
of the implementation to map then to the actual identifiers used by the
underlying directory.
Line 55:
Line 56: /**
Line 57: * Retrieves a group from the directory given its identifier. Note
that this
Line 58: * identifier is the one assigned by the engine to the group, not
the one
Line 80: *
Line 81: * @param query the query
Line 82: * @return a list containing the groups that match the given query
Line 83: */
Line 84: Set<DirectoryGroup> findGroupsByQuery(String query);
We already invented a query language, it is already part of the engine, users
already familiar with it, and already used to generate LDAP queries.
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/users/DirectoryUser.java
Line 15: private String firstName;
Line 16: private String lastName;
Line 17: private String title;
Line 18: private String email;
Line 19: private String department;
I don't see how it will make the implementation simpler. A hash map is not
simpler than six strings. It may make the implementation more flexible, bu not
simpler.
You may think that using a map allows for better generic management of the
attributes, but it doesn't, because when we need to do that we do it mostly
using reflection (except in the GUI, where reflection isn't available) and an a
data structure like a map actually makes things more complex when using
reflection.
We shouldn't also ignore performance, as we frequently do. Look at any heap
dump and you will see that usually the third consumer of memory in the heap are
hash maps, usually after strings and arrays of bytes.
Line 20:
Line 21: // Flag indicating if this user has the administrator role:
Line 22: private boolean isAdmin = false;
Line 23:
--
To view, visit http://gerrit.ovirt.org/15596
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If84a0c9d6553d81cdbbe224972696f169cca90d4
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Juan Hernandez <juan.hernan...@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com>
Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com>
Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com>
Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
