Yair Zaslavsky has posted comments on this change. Change subject: 8. [WIP] core: Introducing Extension manager ......................................................................
Patch Set 21: (7 comments) 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? Done 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() Done 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? Done 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 in Done 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. done. I moved the relevant methods to the parent class. IMHO the code looks more elegant if we still have loadModule method as you suggested, just for loading the method, and one method to take care of instantiation and initialization. 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? done. 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? Done 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
