Alon Bar-Lev has posted comments on this change. Change subject: extensions test tool: logger ......................................................................
Patch Set 4: (41 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 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 of legacy. 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. 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. 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. 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 static Level from Level class. anyway, this also can be in static context, so it is done once when class is loaded. private static final List<String> Levels = ... 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. 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 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.... module = modules.get() if (modules == null) { throw exception } continue the green path 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 can handle: xxx logger --help or any other valid case that skips the logic. 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. 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. 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. 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, as it is expected that we will have more as time goes by. the context should be provided using setContext() method. 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: if (fail) { throw } continue green flow 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. best to have some mapping.... Map such as: "log-record", new Runnable() { @Override void run() { logRecord() } } then you have a method per command and generic logic. here it is overkill, but will be obvious in next modules. 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). 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 ? 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 Line 7: arg.log-file.type = required_argument Line 8: arg.log-level.name = log-level Line 9: arg.log-level.help = file where log will be stored Line 10: arg.log-level.type = required_argument Line 11: arg.log-level.matcher = FINEST|FINER|FINE|CONFIG|INFO|WARNING|SEVERE I think that numeric value is sufficient :))) but ok. Line 12: arg.help.name = help 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? 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 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: LoggerFactory.getLogger(name) this name will be issued within the core logger. 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, or have some special prefix for these classes. 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. 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 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 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 _tmp is something that follows the 'private member' convention, please try to avoid using it for something else. a variable named 's' is sufficient for 'yet another string'. 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. 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 actual. this will allow you to have defaults for everything and reduce logic. 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 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. Thread.currentThread().getContextClassLoader() 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 expected usage output. xxx.arg.yyy.aaa yyy should be sorted 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. 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 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? 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? 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. 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. if { trivial block } else { non trivial block } 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 override these defaults while you parse the arguments? 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. Line 194: mandatory.removeAll(argMap.keySet()); Line 195: if(!mandatory.isEmpty()) { Line 196: throw new IllegalArgumentException( Line 197: "Argument(s) " + StringUtils.join(mandatory, ", ") + " required" https://gerrit.ovirt.org/#/c/37886/4/backend/manager/modules/uutils/src/main/java/org/ovirt/engine/core/uutils/cli/Util.java File backend/manager/modules/uutils/src/main/java/org/ovirt/engine/core/uutils/cli/Util.java: Line 25: typeBox.put(short.class, Short.class); Line 26: typeBox.put(void.class, Void.class); Line 27: } Line 28: Line 29: public static Object getObjectValueByString(Class<?> clazz, String value) { it is more complex than what we need... but ok for now :)) Line 30: Object v = null; Line 31: Line 32: if (clazz.isPrimitive()) { Line 33: clazz = typeBox.get(clazz); -- 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
