Alon Bar-Lev has posted comments on this change. Change subject: 8. [WIP] core: Introducing Extension manager ......................................................................
Patch Set 22: (5 comments) http://gerrit.ovirt.org/#/c/24365/22//COMMIT_MSG Commit Message: Line 7: 8. [WIP] core: Introducing Extension manager Line 8: Line 9: This class is responsible for loading and managing the lifecycle Line 10: of extensions Line 11: please address my comment within commit messages, thanks! Line 12: Change-Id: I182904177ec088e62b35bde870ec79725fabc4e4 http://gerrit.ovirt.org/#/c/24365/22/backend/manager/modules/extension-manager/src/main/java/org/ovirt/engine/core/extensions/mgr/ExtensionManager.java File backend/manager/modules/extension-manager/src/main/java/org/ovirt/engine/core/extensions/mgr/ExtensionManager.java: Line 75: public Map<ExtensionProperties, Object> getContext() { Line 76: return context; Line 77: } Line 78: Line 79: public Extension getExtension() { I would put it as private :) Line 80: return extension; Line 81: } Line 82: Line 83: public void setExtension(Extension extension) { Line 200: throw new ConfigurationException(String.format("The module '%1$s' does not contain the type '%2$s'.", Line 201: module.getIdentifier().getName(), Line 202: extensionEntry.getConfig().getProperty(CLASS))); Line 203: } Line 204: extensionEntry.setExtension(extensionClass.newInstance()); what about setContext? Line 205: extensionEntry.getExtension().init(); Line 206: } catch (InstantiationException | IllegalAccessException | IllegalArgumentException e) { Line 207: throw new ConfigurationException(String.format("Error in instantitating extension based on tye type '%1$'", Line 208: extensionEntry.getConfig().getProperty(CLASS))); Line 215: private void activate() { Line 216: for (ExtensionEntry entry : loadedEntries.values()) { Line 217: if (entry.enabled) { Line 218: activatedEntries.put(entry.getName(), entry); Line 219: createExtension(entry, loadModule(entry.getConfig().getProperty(MODULE))); I do not like createExtension modify the entry... I think it should be: entry.setExtension(createExtension(entry)) there is no reason to load the module here and not within function... also, I am unsure the function of createExtension is worth a separate function but not that important. the activateEntries.put()... should be after initialization is success as last action, this will enable you to skip extensions that fail initialization. you should catch exceptions within the loop, to allow proper initialization without faulty extensions. Line 220: List<ExtensionEntry> entries = providesEntries.get(entry.getProvides()); Line 221: if (entries == null) { Line 222: entries = new ArrayList<>(); Line 223: providesEntries.put(entry.getProvides(), entries); http://gerrit.ovirt.org/#/c/24365/22/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/EngineLocalConfig.java File backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/EngineLocalConfig.java: Line 227: */ Line 228: public List<File> getExtensionsDirectories() { Line 229: String path = getProperty("ENGINE_EXTENSION_PATH"); Line 230: if (path == null) { Line 231: log.warn("No extension configurations directories were provided"); the need of warning is up to caller not this infrastructure component. Line 232: return Collections.emptyList(); Line 233: } Line 234: List<File> results = new ArrayList<File>(); Line 235: for (String currentPath : path.split(":")) { -- To view, visit http://gerrit.ovirt.org/24365 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I182904177ec088e62b35bde870ec79725fabc4e4 Gerrit-PatchSet: 22 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yair Zaslavsky <[email protected]> Gerrit-Reviewer: Alon Bar-Lev <[email protected]> Gerrit-Reviewer: Martin Peřina <[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
