Alon Bar-Lev has posted comments on this change. Change subject: 8. [WIP] core: Introducing Extension manager ......................................................................
Patch Set 21: (8 comments) http://gerrit.ovirt.org/#/c/24365/21//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 add: Topic: AAA Line 12: Change-Id: I182904177ec088e62b35bde870ec79725fabc4e4 http://gerrit.ovirt.org/#/c/24365/21/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 50: props.load(inputStream); Line 51: context.put(ExtensionProperties.CONFIGURATION, props); Line 52: context.put(ExtensionProperties.NAME, props.getProperty(NAME)); Line 53: context.put(ExtensionProperties.PROVIDES, props.getProperty(PROVIDES)); Line 54: Module module = loadModule(props.getProperty(MODULE)); why do you ned module temp variable? Line 55: this.extension = createExtension(module); Line 56: } Line 57: } Line 58: Line 51: context.put(ExtensionProperties.CONFIGURATION, props); Line 52: context.put(ExtensionProperties.NAME, props.getProperty(NAME)); Line 53: context.put(ExtensionProperties.PROVIDES, props.getProperty(PROVIDES)); Line 54: Module module = loadModule(props.getProperty(MODULE)); Line 55: this.extension = createExtension(module); again... this should be called only when activate() Line 56: } Line 57: } Line 58: Line 59: private Extension createExtension(Module module) { Line 58: Line 59: private Extension createExtension(Module module) { Line 60: try { Line 61: ServiceLoader<? extends Extension> serviceLoader = Line 62: ServiceLoader.load(Extension.class, module.getClassLoader()); why do you need serviceLoader temp variable? Line 63: Class<? extends Extension> extensionClass = null; Line 64: for (Extension extension : serviceLoader) { Line 65: if (extension.getClass().getName().equals(getConfig().getProperty(CLASS))) { Line 66: extensionClass = extension.getClass(); Line 71: throw new ConfigurationException(String.format("The module '%1$s' does not contain the type '%2$s'.", Line 72: module.getIdentifier().getName(), Line 73: getConfig().getProperty(CLASS))); Line 74: } Line 75: return extensionClass.newInstance(); I think that where it is created it should also be initialized, think of init() as constructor for interface. Line 76: } catch (InstantiationException | IllegalAccessException | IllegalArgumentException e) { Line 77: throw new ConfigurationException(String.format("Error in instantitating extension based on tye type '%1$'", Line 78: getConfig().getProperty(CLASS))); Line 79: } Line 79: } Line 80: } Line 81: Line 82: private Module loadModule(String moduleSpec) { Line 83: Module module = loadedModules.get(moduleSpec); I think this is quite ugly to use loadedModules within nested class. I think that the entire loading of extension should be in parent at activate(), and ExtensionEntry should have setExtension() or similar to set the instance. Line 84: Line 85: // If the module was not already loaded, load it, get the extension classes from it, and find the proper Line 86: // extension to this extension entry. Line 87: try { Line 81: Line 82: private Module loadModule(String moduleSpec) { Line 83: Module module = loadedModules.get(moduleSpec); Line 84: Line 85: // If the module was not already loaded, load it, get the extension classes from it, and find the proper why is the module = ... is out of the scope of try block? Line 86: // extension to this extension entry. Line 87: try { Line 88: if (module == null) { Line 89: ModuleLoader loader = ModuleLoader.forClass(this.getClass()); Line 90: if (loader == null) { Line 91: throw new ConfigurationException(String.format("The module '%1$s' cannot be loaded as the module system isn't enabled.", Line 92: moduleSpec)); Line 93: } Line 94: ModuleIdentifier id = ModuleIdentifier.fromString(moduleSpec); why do you need id temp variable? Line 95: module = loader.loadModule(id); Line 96: loadedModules.put(moduleSpec, module); Line 97: } Line 98: return module; -- 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: 21 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
