Alon Bar-Lev has posted comments on this change. Change subject: 4. [WIP] core: Introducing configuration loader ......................................................................
Patch Set 2: (12 comments) http://gerrit.ovirt.org/#/c/24365/2/backend/manager/modules/extension-manager/src/main/java/org/ovirt/engine/core/extensions/mgr/ConfigurationLoader.java File backend/manager/modules/extension-manager/src/main/java/org/ovirt/engine/core/extensions/mgr/ConfigurationLoader.java: Line 14: Line 15: /** Line 16: * This class is responsible for loading the required {@code Configuration} in order to create an extension. It holds Line 17: * the logic of ordering and solving conflicts during loading the configuration Line 18: * - Line 19: */ Line 20: public class ConfigurationLoader { Line 21: Line 22: private static final Logger log = LoggerFactory.getLogger(ConfigurationLoader.class); Line 37: } Line 38: return instance; Line 39: } Line 40: Line 41: public List<Configuration> loadFiles(File directory) throws ConfigurationException { loadFiles -> load? Line 42: // Check that the folder that contains the configuration files exists: Line 43: if (!directory.exists()) { Line 44: throw new ConfigurationException( Line 45: "The directory \"" + directory.getAbsolutePath() + "\" containing the configuration files doesn't " Line 40: Line 41: public List<Configuration> loadFiles(File directory) throws ConfigurationException { Line 42: // Check that the folder that contains the configuration files exists: Line 43: if (!directory.exists()) { Line 44: throw new ConfigurationException( FileNotFoundException? Line 45: "The directory \"" + directory.getAbsolutePath() + "\" containing the configuration files doesn't " Line 46: + Line 47: "exist."); Line 48: } Line 53: File[] files = directory.listFiles(); Line 54: if (files != null) { Line 55: sort(files); Line 56: for (File file : files) { Line 57: if (file.getName().endsWith(".conf")) { I think we need .properties here, as it is not really configuration. configuration == similar to what we have at engine.conf.d Line 58: loadFile(file); Line 59: } Line 60: } Line 61: } Line 70: } catch (IOException exception) { Line 71: throw new ConfigurationException( Line 72: "Can't load object configuration file \"" + file.getAbsolutePath() + "\".", Line 73: exception); Line 74: } please wrap the entire function in try/catch, it is very ugly to re-route exceptions this why, unless there is actual meaning to do so... for example, you need to re-route several bad states to different exceptions. in this case IOException within this function is a failure of reading the file at any given point in time. Line 75: Line 76: String name = config.getString(ExtensionManagerKeys.name.getKeyName()); Line 77: Line 78: // Check if the object has been explicitly disabled, if it is then return immediately: Line 71: throw new ConfigurationException( Line 72: "Can't load object configuration file \"" + file.getAbsolutePath() + "\".", Line 73: exception); Line 74: } Line 75: - Line 76: String name = config.getString(ExtensionManagerKeys.name.getKeyName()); Line 77: Line 78: // Check if the object has been explicitly disabled, if it is then return immediately: Line 79: Boolean enabled = config.getBoolean(ExtensionManagerKeys.enabled.getKeyName(), false); Line 72: "Can't load object configuration file \"" + file.getAbsolutePath() + "\".", Line 73: exception); Line 74: } Line 75: Line 76: String name = config.getString(ExtensionManagerKeys.name.getKeyName()); as match as I would like to understand the use of enum... a simple toString() method of enum should be sufficient. Line 77: Line 78: // Check if the object has been explicitly disabled, if it is then return immediately: Line 79: Boolean enabled = config.getBoolean(ExtensionManagerKeys.enabled.getKeyName(), false); Line 80: if (!enabled.booleanValue()) { Line 78: // Check if the object has been explicitly disabled, if it is then return immediately: Line 79: Boolean enabled = config.getBoolean(ExtensionManagerKeys.enabled.getKeyName(), false); Line 80: if (!enabled.booleanValue()) { Line 81: log.debug("The extension \"{}\" is disabled", name); Line 82: return; please do not return at middle of functions. Line 83: } Line 84: Line 85: // Each manager/extension has to have a unique name, in case there is an extension/manager loaded with the same Line 86: // name an error should be logged. Line 80: if (!enabled.booleanValue()) { Line 81: log.debug("The extension \"{}\" is disabled", name); Line 82: return; Line 83: } Line 84: - Line 85: // Each manager/extension has to have a unique name, in case there is an extension/manager loaded with the same Line 86: // name an error should be logged. Line 87: if (configurationNames.contains(name)) { Line 88: log.error("An extension with the name \"{}\" is already loaded", name); Line 85: // Each manager/extension has to have a unique name, in case there is an extension/manager loaded with the same Line 86: // name an error should be logged. Line 87: if (configurationNames.contains(name)) { Line 88: log.error("An extension with the name \"{}\" is already loaded", name); Line 89: return; please do not return at middle of function. please raise exceptions in case of errors, this is why exceptions were introduced. please also add to message the name of the loaded file and the name of the new file, so people can know what happens. Line 90: Line 91: } Line 92: configurationNames.add(name); Line 93: String position = config.getString(ExtensionManagerKeys.position.getKeyName()); Line 88: log.error("An extension with the name \"{}\" is already loaded", name); Line 89: return; Line 90: Line 91: } Line 92: configurationNames.add(name); this should contain, name, file, enabled, order hints. then you can remove the enabled logic from the load method, which is not its task. Line 93: String position = config.getString(ExtensionManagerKeys.position.getKeyName()); Line 94: if (position != null) { Line 95: if (position.equalsIgnoreCase("first")) { Line 96: if (!firstDetected) { Line 108: lastDetected = true; Line 109: } else { Line 110: log.error("There is already an extension that is marked as the last. \"{}\" cannot be set as last", Line 111: name); Line 112: } the logic of reorder should be applied after you load all configurations, we will work on this. for now remove it, it is not important for the tasks we need now. Line 113: } Line 114: } Line 115: } Line 116: -- 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: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yair Zaslavsky <yzasl...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
