Alon Bar-Lev has posted comments on this change. Change subject: extensions test tool: logger ......................................................................
Patch Set 6: (40 comments) looks very good! https://gerrit.ovirt.org/#/c/37886/6/backend/manager/dependencies/common/src/main/modules/org/apache/sshd/main/sshd-core.jar File backend/manager/dependencies/common/src/main/modules/org/apache/sshd/main/sshd-core.jar: ? Line 1: PK Line 2: Line 3: Line 4: ¨/§|¢¶±|¬¼ ô Zùæ° !VLö/üVÞÈYFçf¶¼Ä çàlòËÍP_ATÒÛÖj^¯?<²v 'ýjùK#RÀñ<uÎ÷¢Ã:EUe"/ªåæûDï?ár aÄÒÐ5b4M·¦ ¼×*`+ïus ïÙÂ6_&qÌ5Î:·ü¨¡x ¬þ YIw2:þ&Û§6Ù#Ò<ÓmPXÌ ææ~qbþuÅZ¦ https://gerrit.ovirt.org/#/c/37886/6/backend/manager/extension-tool/backend/manager/extension-tool/src/main/modules/org/ovirt/engine/core/extension-tool/main/module.xml File backend/manager/extension-tool/backend/manager/extension-tool/src/main/modules/org/ovirt/engine/core/extension-tool/main/module.xml: Line 7: </resources> Line 8: Line 9: <dependencies> Line 10: <module name="org.apache.commons.lang" /> Line 11: <module name="org.apache.commons.logging" /> you do not need commons logging. Line 12: <module name="org.ovirt.engine.api.ovirt-engine-extensions-api"/> Line 13: <module name="org.ovirt.engine.core.extensions-manager"/> Line 14: <module name="org.ovirt.engine.core.uutils"/> Line 15: <module name="org.slf4j" /> https://gerrit.ovirt.org/#/c/37886/6/backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/core/CoreUtil.java File backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/core/CoreUtil.java: Line 23: } Line 24: Line 25: public static List<File> listPropertiesFiles(String directory) { Line 26: return listFiles(directory, ".properties"); Line 27: } much better to have extension as parameter and drop this function entirely. Line 28: Line 29: public static List<File> listFiles(String directory, String suffix) { Line 30: File dir = new File(directory); Line 31: List<File> propFiles = new ArrayList<>(); Line 25: public static List<File> listPropertiesFiles(String directory) { Line 26: return listFiles(directory, ".properties"); Line 27: } Line 28: Line 29: public static List<File> listFiles(String directory, String suffix) { if this is called only from core and at one class, better to move it to that class. Line 30: File dir = new File(directory); Line 31: List<File> propFiles = new ArrayList<>(); Line 32: if (!dir.exists()) { Line 33: logger.warn("Directory {0} doesn't exists.", directory); Line 42: } Line 43: } Line 44: Line 45: return propFiles; Line 46: } please remember that --help should print package name, version and local version, this can be done using properties file that is generated during build, see the ldap provider for example, search for config.properties and Config.java, here the config.properties should be config.properties.in Line 47: https://gerrit.ovirt.org/#/c/37886/6/backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/core/ExtensionsToolExecutor.java File backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/core/ExtensionsToolExecutor.java: Line 33: setupLogger(argMap); Line 34: Line 35: List<String> others = (List<String>) argMap.get("__other__"); Line 36: Map<String, ModuleService> moduleServices = loadModules(ModuleService.class); Line 37: if (argMap.containsKey(ARG_HELP) || others.isEmpty()) { I prefer only if --help, if empty issue error that a module was not provided and exit with non zero. Line 38: List<String> modules = new ArrayList<>(moduleServices.keySet()); Line 39: Collections.sort(modules); Line 40: System.out.println("Modules: " + StringUtils.join(modules, ", ")); Line 41: return; Line 37: if (argMap.containsKey(ARG_HELP) || others.isEmpty()) { Line 38: List<String> modules = new ArrayList<>(moduleServices.keySet()); Line 39: Collections.sort(modules); Line 40: System.out.println("Modules: " + StringUtils.join(modules, ", ")); Line 41: return; please avoid return in middle of functions. Line 42: } Line 43: String module = others.remove(0); Line 44: ModuleService moduleService = moduleServices.get(module); Line 45: if (moduleService == null) { Line 43: String module = others.remove(0); Line 44: ModuleService moduleService = moduleServices.get(module); Line 45: if (moduleService == null) { Line 46: throw new IllegalArgumentException( Line 47: String.format("No such '%1$s' module exists. To list existing modules use --help argument.", module) not sure I like the --help note here... but ok :) Line 48: ); Line 49: } Line 50: if(moduleService.parseArguments(others)) { Line 51: System.out.println(module + " help"); // TODO: help printer Line 46: throw new IllegalArgumentException( Line 47: String.format("No such '%1$s' module exists. To list existing modules use --help argument.", module) Line 48: ); Line 49: } Line 50: if(moduleService.parseArguments(others)) { what is the meaning of it? true for help? there should be something more explicit. Line 51: System.out.println(module + " help"); // TODO: help printer Line 52: return; Line 53: } Line 54: Line 51: System.out.println(module + " help"); // TODO: help printer Line 52: return; Line 53: } Line 54: Line 55: loadExtensions(moduleService, (String) argMap.get("extensions-dir")); we should have both options of entire directory or specific files Line 56: moduleService.run(); Line 57: } catch (IllegalArgumentException ex) { Line 58: System.out.println(ex.getMessage()); Line 59: logger.error(ex.getMessage()); Line 55: loadExtensions(moduleService, (String) argMap.get("extensions-dir")); Line 56: moduleService.run(); Line 57: } catch (IllegalArgumentException ex) { Line 58: System.out.println(ex.getMessage()); Line 59: logger.error(ex.getMessage()); debug stack as well Line 60: } catch (Throwable t) { Line 61: System.out.println(t.getMessage()); Line 62: logger.error(t.getMessage()); Line 63: logger.debug(t.getMessage(), t); Line 57: } catch (IllegalArgumentException ex) { Line 58: System.out.println(ex.getMessage()); Line 59: logger.error(ex.getMessage()); Line 60: } catch (Throwable t) { Line 61: System.out.println(t.getMessage()); System.err Line 62: logger.error(t.getMessage()); Line 63: logger.debug(t.getMessage(), t); Line 64: } Line 65: } Line 59: logger.error(ex.getMessage()); Line 60: } catch (Throwable t) { Line 61: System.out.println(t.getMessage()); Line 62: logger.error(t.getMessage()); Line 63: logger.debug(t.getMessage(), t); I am unsure what is the difference between both catches... also, if t.getMessage() is null, print t.getClass().getName(). test using null pointer exception. Line 64: } Line 65: } Line 66: Line 67: private static void loadExtensions(ModuleService moduleService, String dir) { Line 68: Map<String, ExtensionProxy> proxies = new HashMap<>(); Line 69: ExtMap context = new ExtMap().mput(Base.GlobalContextKeys.EXTENSIONS_MAP, proxies); Line 70: ExtensionsManager extensionsManager = new ExtensionsManager(); Line 71: for(File f : CoreUtil.listPropertiesFiles(dir)) { Line 72: String proxy = extensionsManager.load(f); no need for proxy temp variable Line 73: proxies.put(proxy, extensionsManager.getExtensionByName(proxy)); Line 74: } Line 75: moduleService.setContext(context); Line 76: for(String proxy : proxies.keySet()) { Line 71: for(File f : CoreUtil.listPropertiesFiles(dir)) { Line 72: String proxy = extensionsManager.load(f); Line 73: proxies.put(proxy, extensionsManager.getExtensionByName(proxy)); Line 74: } Line 75: moduleService.setContext(context); this should be as early as we load modules. Line 76: for(String proxy : proxies.keySet()) { Line 77: extensionsManager.initialize(proxy); Line 78: } Line 79: } Line 96: } Line 97: Line 98: return new ParametersParser( Line 99: CoreUtil.loadProperties(ExtensionsToolExecutor.class, "arguments.properties"), Line 100: CoreUtil.loadProperties(ExtensionsToolExecutor.class, "defaults.properties") the defaults should be loaded by the parameters parser not here, or I may not understand yet what these defaults are :) Line 101: ).parse(args); Line 102: } Line 103: Line 104: private static void setupLogger(Map<String, Object> args) throws IOException{ Line 108: if(logfile != null) { Line 109: FileHandler fh = new FileHandler(logfile); Line 110: fh.setFormatter(new SimpleFormatter()); Line 111: logger.addHandler(fh); Line 112: } please make sure that --log-level=ALL issues all FINE/FINEST/DEBUG etc to both console and log file. Line 113: } https://gerrit.ovirt.org/#/c/37886/6/backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/logger/services/LoggerServiceImpl.java File backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/logger/services/LoggerServiceImpl.java: Line 23: private Map<String, Object> args; Line 24: private Map<String, Runnable> actions; Line 25: Line 26: public LoggerServiceImpl() { Line 27: args = new HashMap<>(); no need. private Map<String, Object> args = new HashMap<>(); Line 28: actions = new HashMap<>(); Line 29: actions.put( Line 30: "log-record", Line 31: new Runnable() { Line 33: public void run() { Line 34: logrecord(); Line 35: } Line 36: } Line 37: ); this should be at static context. private static Map<String, Runnable> actions = new HashMap<>(); static { actions.put(); } Line 38: } Line 39: Line 40: @Override Line 41: public String getName() { Line 39: Line 40: @Override Line 41: public String getName() { Line 42: return "logger"; Line 43: } we probably need description as well... for nice usage :) Line 44: Line 45: @Override Line 46: public void setContext(ExtMap context) { Line 47: this.context = context; Line 50: @Override Line 51: public boolean parseArguments(List<String> argsList) throws Exception { Line 52: ParametersParser parser = new ParametersParser( Line 53: CoreUtil.loadProperties(getClass(), "arguments.properties"), Line 54: CoreUtil.loadProperties(getClass(), "defaults.properties") still trying to understand what are these defaults. Line 55: ); Line 56: args = parser.parse(argsList); Line 57: Line 58: return args.containsKey("help"); Line 54: CoreUtil.loadProperties(getClass(), "defaults.properties") Line 55: ); Line 56: args = parser.parse(argsList); Line 57: Line 58: return args.containsKey("help"); better to have a separate method, boolean for help is unexpected. Line 59: } Line 60: Line 61: @Override Line 62: public void run() throws Exception { Line 61: @Override Line 62: public void run() throws Exception { Line 63: List<String> argsList = (List <String>)args.get("__other__"); Line 64: String action = argsList.remove(0); Line 65: if(!actions.containsKey(action)) { just get action and check if null, no reason to get twice Line 66: throw new IllegalArgumentException( Line 67: String.format("No such action '%1$s' exists for module '%2$s'", action, getName()) Line 68: ); Line 69: } https://gerrit.ovirt.org/#/c/37886/6/backend/manager/extension-tool/src/main/resources/org/ovirt/engine/exttool/core/arguments.properties File backend/manager/extension-tool/src/main/resources/org/ovirt/engine/exttool/core/arguments.properties: Line 1: core.arg.extensions-dir.name = extensions-dir Line 2: core.arg.extensions-dir.help = path to directory of extensions Line 3: core.arg.extensions-dir.type = required_argument Line 4: core.arg.extensions-dir.default = /etc/ovirt-engine/extensions.d/ this file should be converted to template (add .in suffix, manage Makefile stuff) and add here @ENGINE_ETC@/extensions.d Line 5: core.arg.log-file.name = log-file Line 6: core.arg.log-file.help = file where log will be stored Line 7: core.arg.log-file.type = required_argument Line 8: core.arg.log-level.name = log-level Line 9: core.arg.log-level.help = file where log will be stored Line 10: core.arg.log-level.type = required_argument Line 11: core.arg.log-level.convert = java.util.logging.Level Line 12: core.arg.log-level.default = INFO Line 13: core.arg.log-level.matcher = FINEST|FINER|FINE|CONFIG|INFO|WARNING|SEVERE ALL as well Line 14: core.arg.help.name = help Line 11: core.arg.log-level.convert = java.util.logging.Level Line 12: core.arg.log-level.default = INFO Line 13: core.arg.log-level.matcher = FINEST|FINER|FINE|CONFIG|INFO|WARNING|SEVERE Line 14: core.arg.help.name = help Line 15: core.arg.help.help = show possible modules you can use show this output / this text / this help it should show usage including any other options. https://gerrit.ovirt.org/#/c/37886/6/backend/manager/extension-tool/src/main/resources/org/ovirt/engine/exttool/core/defaults.properties File backend/manager/extension-tool/src/main/resources/org/ovirt/engine/exttool/core/defaults.properties: Line 1: core.arg.mandatory = false this should have no prefix :) so it will be common to all. it should be in the class/package/namespace of the parser and always loaded. Line 2: core.arg.help = no help for this argument Line 3: core.arg.type = no_argument Line 4: core.arg.convert = java.lang.String Line 5: core.arg.default = Line 3: core.arg.type = no_argument Line 4: core.arg.convert = java.lang.String Line 5: core.arg.default = Line 6: core.arg.matcher = .* Line 7: core.arg.metavar = STRING please make sure you have new lines https://gerrit.ovirt.org/#/c/37886/6/backend/manager/extension-tool/src/main/resources/org/ovirt/engine/exttool/logger/services/arguments.properties File backend/manager/extension-tool/src/main/resources/org/ovirt/engine/exttool/logger/services/arguments.properties: Line 16: logrecord.arg.level.matcher = FINEST|FINER|FINE|CONFIG|INFO|WARNING|SEVERE Line 17: logrecord.arg.level.convert = java.util.logging.Level Line 18: logrecord.arg.level.default = INFO Line 19: logrecord.arg.help.name = help Line 20: logrecord.arg.help.help = show help for logger module we probably need to add positional parameter usage as well, not critical now. https://gerrit.ovirt.org/#/c/37886/6/backend/manager/extension-tool/src/main/resources/org/ovirt/engine/exttool/logger/services/defaults.properties File backend/manager/extension-tool/src/main/resources/org/ovirt/engine/exttool/logger/services/defaults.properties: not required Line 1: logrecord.arg.mandatory = false Line 2: logrecord.arg.help = no help for this argument Line 3: logrecord.arg.type = no_argument Line 4: logrecord.arg.convert = java.lang.String https://gerrit.ovirt.org/#/c/37886/6/backend/manager/modules/extensions-api-root/extensions-api/src/main/java/org/ovirt/engine/api/extensions/Base.java File backend/manager/modules/extensions-api-root/extensions-api/src/main/java/org/ovirt/engine/api/extensions/Base.java: please explain why base is effected Line 1: package org.ovirt.engine.api.extensions; Line 2: Line 3: import java.util.Collection; Line 4: import java.util.Map; https://gerrit.ovirt.org/#/c/37886/6/backend/manager/modules/uutils/src/main/java/org/ovirt/engine/core/uutils/cli/parser/ParametersParser.java File backend/manager/modules/uutils/src/main/java/org/ovirt/engine/core/uutils/cli/parser/ParametersParser.java: Line 23: private Map<String, ParserArgument> arguments; Line 24: private Set<String> mandatory; Line 25: private List<String> defaults; Line 26: Line 27: public ParametersParser(Properties properties, Properties defaultPropeties) { you should load the defaults into static. then when loading the other properties use the default as default, so maybe better to provide here a Reader and not Properties and construct it using the default. Line 28: this.arguments = new HashMap<>(); Line 29: this.mandatory = new HashSet<>(); Line 30: this.defaults = new ArrayList<>(); Line 31: parseProperties(properties, defaultPropeties); Line 26: Line 27: public ParametersParser(Properties properties, Properties defaultPropeties) { Line 28: this.arguments = new HashMap<>(); Line 29: this.mandatory = new HashSet<>(); Line 30: this.defaults = new ArrayList<>(); initialize directly at class level. Line 31: parseProperties(properties, defaultPropeties); Line 32: } Line 33: Line 34: static enum ArgumentType { Line 66: others.add(arg); Line 67: break; Line 68: } Line 69: Line 70: String[] argVal = parseArgument(arg.substring(2)); please set key and value or name and value and not use indexes all over. Line 71: ParserArgument parserArgument = arguments.get(argVal[0]); Line 72: if(parserArgument == null) { Line 73: throw new IllegalArgumentException( Line 74: String.format("Argument %1$s is not valid", arg) Line 73: throw new IllegalArgumentException( Line 74: String.format("Argument %1$s is not valid", arg) Line 75: ); Line 76: } Line 77: if(parserArgument.getType() == ArgumentType.required_argument && argVal[1] == null) { maybe you like to manage: --p v --p=v code is becoming good shape so we can do something like: arg = args.remove(0) ... key, value = arg.split("="); if (value == null && (arg.type == optional_args || arg.type ==required_args) { value = args.remove(0); if (value == "--") { value = null; stop = true; } } if (value == null && arg.type == required) { error. } Line 78: throw new IllegalArgumentException( Line 79: String.format("Argument's '%1$s' value is required", argVal[0]) Line 80: ); Line 81: } Line 82: Matcher m = parserArgument.getMatcher().matcher(argVal[1]); Line 83: if (!m.find()) { Line 84: throw new IllegalArgumentException( Line 85: String.format( Line 86: "Argument's '%1$s' value don't match pattern: ", parserArgument.getMatcher().pattern() you already have m Line 87: ) Line 88: ); Line 89: } Line 90: Line 91: if(!argMap.containsKey(argVal[0])) { Line 92: argMap.put( Line 93: argVal[0], Line 94: Util.getObjectValueByString(parserArgument.getConvert(), argVal[1]) Line 95: ); if you ignore value, then better print error, but usually last wins. anyway, we need to add a type of parameter which will be an array... if it is the type then: --xxx=a1 --xxx=a2 will result in xxx -> [a1, a2] Line 96: } Line 97: } Line 98: mandatory.removeAll(argMap.keySet()); Line 99: if(!mandatory.isEmpty()) { Line 94: Util.getObjectValueByString(parserArgument.getConvert(), argVal[1]) Line 95: ); Line 96: } Line 97: } Line 98: mandatory.removeAll(argMap.keySet()); you change object state which is not expected in this case, as mandatory should be fixed one constructed, so copy it and then use. Line 99: if(!mandatory.isEmpty()) { Line 100: throw new IllegalArgumentException( Line 101: String.format("Argument(s) '%1$s' required", StringUtils.join(mandatory, ", ")) Line 102: ); Line 118: String.format("Name of argument '%1$s' have to specified", key[2]) Line 119: ); Line 120: } Line 121: argument.setMetavar( Line 122: getArgAttrValue(properties, defaultProperties, key[0], key[2], "metavar") in any case you should have single properties instance. Line 123: ); Line 124: argument.setHelp( Line 125: getArgAttrValue(properties, defaultProperties, key[0], key[2], "help") Line 126: ); Line 166: arguments.put(key[2], argument); Line 167: } Line 168: } Line 169: Line 170: private Set<String[]> getUsageArguments(Properties properties) { won't map with key value be better? not sure I understand exactly what it does. but it does strange and the usage above is ugly (with the indexes). Line 171: SortedSet<String[]> args = new TreeSet<>(new Comparator<String[]>() { Line 172: @Override Line 173: public int compare(String[] o, String[] t1) { Line 174: return o[2].compareTo(t1[2]); -- To view, visit https://gerrit.ovirt.org/37886 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie06113c5d56a49e58d557c851f9ff00b9a9ca409 Gerrit-PatchSet: 6 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ondřej Macháček <machacek.on...@gmail.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Ondra Machacek <omach...@redhat.com> Gerrit-Reviewer: Ondřej Macháček <machacek.on...@gmail.com> Gerrit-Reviewer: automat...@ovirt.org 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
