Ondřej Macháček has posted comments on this change. Change subject: extensions test tool: logger ......................................................................
Patch Set 4: (39 comments) https://gerrit.ovirt.org/#/c/37886/4/Makefile File Makefile: Line 33: BUILD_UT=0 Line 34: EXTRA_BUILD_FLAGS= Line 35: BUILD_VALIDATION=0 Line 36: BUILD_ENV_VALIDATION=0 Line 37: BUILD_JAVA_OPTS_MAVEN?=-XX:MaxPermSize=1024M > you should not change this while you work... you can just write make XXX=0 Oh, I commited this accidentally, but thanks for tip. Line 38: BUILD_JAVA_OPTS_GWT?= Line 39: DEV_REBUILD=1 Line 40: DEV_BUILD_GWT_DRAFT=0 Line 41: DEV_EXTRA_BUILD_FLAGS= https://gerrit.ovirt.org/#/c/37886/4/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 9: <dependencies> Line 10: <module name="org.apache.commons.lang" /> Line 11: <module name="org.ovirt.engine.api.ovirt-engine-extensions-api"/> Line 12: <module name="org.ovirt.engine.core.extensions-manager"/> Line 13: <module name="org.ovirt.engine.core.utils"/> > you should not use anything at utils, this is very bad module for us, lots Done Line 14: <module name="org.ovirt.engine.core.uutils"/> Line 15: <module name="org.slf4j"/> Line 16: </dependencies> https://gerrit.ovirt.org/#/c/37886/4/backend/manager/extension-tool/pom.xml File backend/manager/extension-tool/pom.xml: Line 38: <dependency> Line 39: <groupId>${engine.groupId}</groupId> Line 40: <artifactId>utils</artifactId> Line 41: <version>${engine.version}</version> Line 42: </dependency> > same, do not use utils. Done Line 43: <dependency> Line 44: <groupId>org.slf4j</groupId> Line 45: <artifactId>slf4j-api</artifactId> Line 46: <version>${slf4j.version}</version> https://gerrit.ovirt.org/#/c/37886/4/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 17: } Line 18: Line 19: public static Properties loadProperties(Class<?> clazz, String resource) throws IOException{ Line 20: Properties prop = new Properties(); Line 21: InputStream is = clazz.getResourceAsStream(resource); > always use try with resources. Done Line 22: if(is.available() > 0) { Line 23: prop.load(is); Line 24: } Line 25: Line 19: public static Properties loadProperties(Class<?> clazz, String resource) throws IOException{ Line 20: Properties prop = new Properties(); Line 21: InputStream is = clazz.getResourceAsStream(resource); Line 22: if(is.available() > 0) { Line 23: prop.load(is); > do not load input stream as it does not support utf-8 use Reader. Done Line 24: } Line 25: Line 26: return prop; Line 27: } Line 25: Line 26: return prop; Line 27: } Line 28: Line 29: public static List<String> getLoggingLevels() { > not sure why you need this, but best to use reflection... extract all stati Don't need anymore. Line 30: return Arrays.asList( Line 31: Level.FINEST.getName(), Line 32: Level.FINER.getName(), Line 33: Level.FINE.getName(), https://gerrit.ovirt.org/#/c/37886/4/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 1: package org.ovirt.engine.exttool.core; Line 2: Line 3: import org.apache.commons.lang.StringUtils; Line 4: import org.ovirt.engine.core.extensions.mgr.ExtensionsManager; Line 5: import org.ovirt.engine.core.utils.log.JavaLoggingUtils; > please do not use this. Done Line 6: import org.ovirt.engine.core.uutils.cli.ParametersParser; Line 7: Line 8: import java.io.File; Line 9: import java.util.*; Line 23: System.out.println(getUsage(moduleServices.keySet())); // FIXME: Line 24: return; Line 25: } Line 26: Line 27: Deque<String> others = (Deque<String>)argMap.get("__other__"); > better use simplest interface, in this case ordered is List Done Line 28: String module = others.removeFirst(); Line 29: ModuleService moduleService = moduleServices.get(module); Line 30: Line 31: if(moduleService != null) { Line 27: Deque<String> others = (Deque<String>)argMap.get("__other__"); Line 28: String module = others.removeFirst(); Line 29: ModuleService moduleService = moduleServices.get(module); Line 30: Line 31: if(moduleService != null) { > always negative first.... Done Line 32: moduleService.parseArguments(others); Line 33: ExtensionsManager extensionsManager = new ExtensionsManager(); Line 34: File config = new File((String)argMap.get(ARG_EXT_CONFIG)); Line 35: if (!config.exists()) { Line 28: String module = others.removeFirst(); Line 29: ModuleService moduleService = moduleServices.get(module); Line 30: Line 31: if(moduleService != null) { Line 32: moduleService.parseArguments(others); > this should return something or another method added to get status, so we c Done Line 33: ExtensionsManager extensionsManager = new ExtensionsManager(); Line 34: File config = new File((String)argMap.get(ARG_EXT_CONFIG)); Line 35: if (!config.exists()) { Line 36: throw new Exception("Could not find extension configuration at the specified path"); Line 30: Line 31: if(moduleService != null) { Line 32: moduleService.parseArguments(others); Line 33: ExtensionsManager extensionsManager = new ExtensionsManager(); Line 34: File config = new File((String)argMap.get(ARG_EXT_CONFIG)); > we need a directory or worse multiple configs. For now I added dir. Line 35: if (!config.exists()) { Line 36: throw new Exception("Could not find extension configuration at the specified path"); Line 37: } Line 38: extensionsManager.initialize(extensionsManager.load(config)); Line 34: File config = new File((String)argMap.get(ARG_EXT_CONFIG)); Line 35: if (!config.exists()) { Line 36: throw new Exception("Could not find extension configuration at the specified path"); Line 37: } Line 38: extensionsManager.initialize(extensionsManager.load(config)); > you should first load all then initialize all. Done Line 39: moduleService.run(extensionsManager); Line 40: } else { Line 41: System.out.println("No such " + module + " module exists. To list existing modules use --help argument."); Line 42: } https://gerrit.ovirt.org/#/c/37886/4/backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/core/ModuleService.java File backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/core/ModuleService.java: Line 1: package org.ovirt.engine.exttool.core; Line 2: Line 3: import org.ovirt.engine.core.extensions.mgr.ExtensionsManager; Line 4: Line 5: import java.util.Deque; > the most primitive form, List is sufficient in this case. Done Line 6: Line 7: public interface ModuleService { Line 8: Line 9: public void parseArguments(Deque<String> args) throws Exception; Line 7: public interface ModuleService { Line 8: Line 9: public void parseArguments(Deque<String> args) throws Exception; Line 10: Line 11: public void run(ExtensionsManager extensionsManager) throws Exception; > please provide a context, and within that context put extensions manager, a Done Line 12: Line 13: public String getName(); https://gerrit.ovirt.org/#/c/37886/4/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 48: } else { Line 49: throw new IllegalArgumentException( Line 50: "No action specified for module " + getName() Line 51: ); Line 52: } > exceptional pattern: Done Line 53: Line 54: LogRecord logRecord = new LogRecord( Line 55: (Level)argMap.get("level"), Line 56: (String)argMap.get("message") Line 50: "No action specified for module " + getName() Line 51: ); Line 52: } Line 53: Line 54: LogRecord logRecord = new LogRecord( > please separate to own function. Done Line 55: (Level)argMap.get("level"), Line 56: (String)argMap.get("message") Line 57: ); Line 58: logRecord.setLoggerName((String)argMap.get("logger-name")); Line 81: ); Line 82: } Line 83: Line 84: @Override Line 85: public String getName() { > please put this first (after constructor). Done Line 86: return "logger"; Line 87: } https://gerrit.ovirt.org/#/c/37886/4/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: arg.extension-config.name = extension-config > extensions-conf.d or extesions-dir ? Done Line 2: arg.extension-config.help = path to directory of extensions Line 3: arg.extension-config.type = required_argument Line 4: arg.extension-config.default = /etc/ovirt-engine/extensions.d/ Line 5: arg.log-file.name = log-file https://gerrit.ovirt.org/#/c/37886/4/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 1: arg.logger-name.name = extension-config > why extension-config? it's copy-pasted from second one properties, forgot to change. Line 2: arg.logger-name.mandatory = true Line 3: arg.logger-name.help = path to directory of extensions Line 4: arg.logger-name.type = required_argument Line 5: arg.message.name = log-file Line 1: arg.logger-name.name = extension-config Line 2: arg.logger-name.mandatory = true Line 3: arg.logger-name.help = path to directory of extensions > this should be in the core also forgot to change. Line 4: arg.logger-name.type = required_argument Line 5: arg.message.name = log-file Line 6: arg.message.help = file where log will be stored Line 7: arg.message.type = required_argument Line 1: arg.logger-name.name = extension-config Line 2: arg.logger-name.mandatory = true Line 3: arg.logger-name.help = path to directory of extensions Line 4: arg.logger-name.type = required_argument Line 5: arg.message.name = log-file > we do not need log file, just name as in: same as above Line 6: arg.message.help = file where log will be stored Line 7: arg.message.type = required_argument Line 8: arg.message.mandatory = true Line 9: arg.level.name = level https://gerrit.ovirt.org/#/c/37886/4/backend/manager/modules/uutils/src/main/java/org/ovirt/engine/core/uutils/cli/ArgumentType.java File backend/manager/modules/uutils/src/main/java/org/ovirt/engine/core/uutils/cli/ArgumentType.java: Line 1: package org.ovirt.engine.core.uutils.cli; > please move this to cli.<something> so we can avoid mixing this and the old Done Line 2: Line 3: public enum ArgumentType { Line 4: Line 5: required_argument("required_argument"), Line 1: package org.ovirt.engine.core.uutils.cli; Line 2: Line 3: public enum ArgumentType { > this can be nested class. Done Line 4: Line 5: required_argument("required_argument"), Line 6: optional_argument("optional_argument"), Line 7: no_argument("no_argument"); https://gerrit.ovirt.org/#/c/37886/4/backend/manager/modules/uutils/src/main/java/org/ovirt/engine/core/uutils/cli/ParametersParser.java File backend/manager/modules/uutils/src/main/java/org/ovirt/engine/core/uutils/cli/ParametersParser.java: Line 13: import java.util.Set; Line 14: import java.util.regex.Matcher; Line 15: import java.util.regex.Pattern; Line 16: Line 17: public class ParametersParser { > indent Done Line 18: Line 19: private static final String LONG_PREFIX = "--"; Line 20: Line 21: private Map<String, ParserArgument> arguments; Line 18: Line 19: private static final String LONG_PREFIX = "--"; Line 20: Line 21: private Map<String, ParserArgument> arguments; Line 22: private Properties properties; > no reason to keep this Done Line 23: Line 24: public ParametersParser(Properties properties) { Line 25: this.properties = properties; Line 26: this.arguments = new HashMap<>(); Line 28: } Line 29: Line 30: private void parseProperties() { Line 31: String _tmp; Line 32: ParserArgument argument; > no reason to define these at top level Done Line 33: for(String name : getUsageArguments()) { Line 34: argument = new ParserArgument(); Line 35: Line 36: argument.setName(getArgAttrValue(name, "name")); Line 34: argument = new ParserArgument(); Line 35: Line 36: argument.setName(getArgAttrValue(name, "name")); Line 37: if(argument.getName() == null) { Line 38: throw new IllegalArgumentException("Name of argument " + name + " have to specified"); > please use String.format() to format messages, do not concat strings. Done Line 39: } Line 40: Line 41: _tmp = getArgAttrValue(name, "type"); Line 42: argument.setType( Line 40: Line 41: _tmp = getArgAttrValue(name, "type"); Line 42: argument.setType( Line 43: _tmp != null ? ArgumentType.valueOf(_tmp) : ArgumentType.no_argument Line 44: ); > please use default properties file, and load the default and then the actua Done Line 45: Line 46: argument.setHelp(getArgAttrValue(name, "help")); Line 47: Line 48: _tmp = getArgAttrValue(name, "matcher"); Line 47: Line 48: _tmp = getArgAttrValue(name, "matcher"); Line 49: if(_tmp != null) { Line 50: argument.setMatcher(Pattern.compile(_tmp)); Line 51: } > default matcher can be .* to reduce logic Done Line 52: Line 53: argument.setMandatory( Line 54: Boolean.parseBoolean(getArgAttrValue(name, "mandatory")) Line 55: ); Line 56: Line 57: _tmp = getArgAttrValue(name, "convert"); Line 58: if(_tmp != null) { Line 59: try { Line 60: argument.setConvert(Class.forName(_tmp)); > Please use the context classloader to locate the class. Done Line 61: } catch (ClassNotFoundException ex) { Line 62: throw new IllegalArgumentException("Can't convert argument: " + name + ". Class not found: " + _tmp); Line 63: } Line 64: } Line 71: } Line 72: } Line 73: Line 74: Line 75: private Set<String> getUsageArguments() { > we will eventually need to sort based on the 3rtd component so we produce e Done Line 76: Set<String> args = new HashSet<>(); Line 77: for (String argName : properties.stringPropertyNames()) { Line 78: String[] param = argName.split("\\."); Line 79: if (param.length < 3) { Line 92: public Map<String, Object> parse(List<String> args) throws Exception { Line 93: return parse(new ArrayDeque<String>(args)); Line 94: } Line 95: Line 96: public Map<String, Object> parse(Deque<String> args) throws Exception { > no need for Deque, List should be sufficient. Done Line 97: Map<String, Object> argMap = new HashMap<>(); Line 98: Deque<String> others = new ArrayDeque<>(); Line 99: boolean firstNonParameter = false; Line 100: Line 107: if(arg.equals(LONG_PREFIX)) { Line 108: break; Line 109: } Line 110: if(arg.startsWith(LONG_PREFIX)) { Line 111: String[] _arg = parseArgument(arg.substring(2)); > no _ prefix please Done Line 112: ParserArgument parserArgument = arguments.get(_arg[0]); Line 113: if(parserArgument == null) { Line 114: others.addLast(arg); Line 115: continue; Line 110: if(arg.startsWith(LONG_PREFIX)) { Line 111: String[] _arg = parseArgument(arg.substring(2)); Line 112: ParserArgument parserArgument = arguments.get(_arg[0]); Line 113: if(parserArgument == null) { Line 114: others.addLast(arg); > why? this should be syntax error, no? No, because if you parse core, the rest of args will be stored in __other__, like 'logger-name, level, message', the core don't undertand, thus add it to __other__, but logger later will recognize. Line 115: continue; Line 116: } Line 117: Line 118: if(parserArgument.getDefaultValue() != null && _arg[1] == null) { Line 116: } Line 117: Line 118: if(parserArgument.getDefaultValue() != null && _arg[1] == null) { Line 119: _arg[1] = String.valueOf(parserArgument.getDefaultValue()); Line 120: } > I do not understand this. you already filled the defaults, no? Correct, moved. Line 121: Line 122: if(parserArgument.getType() == ArgumentType.required_argument && _arg[1] == null) { Line 123: throw new IllegalArgumentException( Line 124: "Argument's '" + _arg[0] + "' value is reguired" Line 136: Line 137: // convert value Line 138: if(parserArgument.getConvert() != null) { Line 139: argConverted = Util.getObjectValueByString(parserArgument.getConvert(), _arg[1]); Line 140: } > converter class can be default java.lang.String and reduce logic. Done Line 141: Line 142: argMap.put( Line 143: _arg[0], Line 144: argConverted == null ? _arg[1] : argConverted Line 149: } else { Line 150: others.addLast(arg); Line 151: } Line 152: firstNonParameter = true; Line 153: } > not sure what this else is doing, but should be first within condition. Done Line 154: } Line 155: // Check all mandatory specified. Line 156: checkMandatory(argMap); Line 157: Line 180: if(!defaults.isEmpty()) { Line 181: for(String s : defaults) { Line 182: args.addLast("--" + s); Line 183: } Line 184: } > I do not understand... why don't you just put the defaults in the map, then Done Line 185: } Line 186: Line 187: private void checkMandatory(Map<String, Object> argMap) { Line 188: Set<String> mandatory = new HashSet<>(); Line 189: for(String arg : arguments.keySet()) { Line 190: if(arguments.get(arg).isMandatory()) { Line 191: mandatory.add(arg); Line 192: } Line 193: } > this you can prepare ahead when parsing properties. Done Line 194: mandatory.removeAll(argMap.keySet()); Line 195: if(!mandatory.isEmpty()) { Line 196: throw new IllegalArgumentException( Line 197: "Argument(s) " + StringUtils.join(mandatory, ", ") + " required" -- 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: 4 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: 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
