Yair Zaslavsky has posted comments on this change. Change subject: aaa: Chages to ExtensionsManager ......................................................................
Patch Set 3: (4 comments) http://gerrit.ovirt.org/#/c/27785/3/backend/manager/modules/extensions-manager/src/main/java/org/ovirt/engine/core/extensions/mgr/ExtensionsManager.java File backend/manager/modules/extensions-manager/src/main/java/org/ovirt/engine/core/extensions/mgr/ExtensionsManager.java: Line 120: private Map<String, BindingsLoader> bindingsLoaders = new HashMap<>(); Line 121: private Map<String, ExtensionEntry> loadedEntries = new HashMap<>(); Line 122: private ExtMap globalContext = new ExtMap().mput(Base.GlobalContextKeys.EXTENSIONS, new ArrayList<ExtMap>()); Line 123: Line 124: private Properties configProperties = new Properties(); > can you please explain why we need this? We would like utils to depend on extension-manager. However, extension-manager , prior to this patch, depends on utils. To avoid circular dependency, I removed all references to EngineLocalConfig. As you can see later in the code (line 228) - I am querying these properties in order to find all extensions that were set to enabled at configuration. Line 125: Line 126: public ExtMap getGlobalContext() { Line 127: return globalContext; Line 128: } Line 145: } Line 146: return result; Line 147: } Line 148: Line 149: public ExtensionsManager(Map<String, String> properties, List<File> extensionsDirectories, String applicationName) { > why not just have empty? In the extension tool I would also like to perform such an iteration, don't you think this is code duplication? Do you see a case where ExtensionsManager should not load configurations from file? Line 150: bindingsLoaders.put(Base.ConfigBindingsMethods.JBOSSMODULE, new JBossBindingsLoader()); Line 151: this.configProperties.putAll(properties); Line 152: globalContext.put(Base.GlobalContextKeys.APPLICATION_NAME, applicationName); Line 153: http://gerrit.ovirt.org/#/c/27785/3/backend/manager/modules/pom.xml File backend/manager/modules/pom.xml: Line 17: <modules> Line 18: <module>extensions-api-root</module> Line 19: <module>uutils</module> Line 20: <module>compat</module> Line 21: <module>extensions-manager</module> > you can move this even above the compat OK. Line 22: <module>utils</module> Line 23: <module>common</module> Line 24: <module>dal</module> Line 25: <module>vdsbroker</module> http://gerrit.ovirt.org/#/c/27785/3/backend/manager/modules/utils/pom.xml File backend/manager/modules/utils/pom.xml: Line 73: <dependency> Line 74: <groupId>${engine.groupId}</groupId> Line 75: <artifactId>extensions-manager</artifactId> Line 76: <version>${engine.version}</version> Line 77: </dependency> > why one above and one bellow? Done Line 78: Line 79: <dependency> Line 80: <groupId>javax.transaction</groupId> Line 81: <artifactId>jta</artifactId> -- To view, visit http://gerrit.ovirt.org/27785 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1c914df29a0dbf52ff6d2f8149687b31b4faffe1 Gerrit-PatchSet: 3 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yair Zaslavsky <[email protected]> Gerrit-Reviewer: Alon Bar-Lev <[email protected]> Gerrit-Reviewer: Yair Zaslavsky <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
