Ondřej Macháček has posted comments on this change. Change subject: extensions test tool: logger ......................................................................
Patch Set 19: (16 comments) https://gerrit.ovirt.org/#/c/37886/19/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 32: Line 33: public static void main(String[] args) { Line 34: args = new String[] {"logger", "log-record", "--help"}; Line 35: int exitStatus = 1; Line 36: Map<String, ModuleService> moduleServices = loadModules(ModuleService.class); > please do this after you set up logger, within the try block. Done Line 37: try { Line 38: ParametersParser parser = new ParametersParser( Line 39: ExtensionsToolExecutor.class.getResourceAsStream("arguments.properties"), Line 40: "core" Line 39: ExtensionsToolExecutor.class.getResourceAsStream("arguments.properties"), Line 40: "core" Line 41: ); Line 42: Map<String,Object> argMap = parser.parse(args); Line 43: setupLogger(argMap); > again... setupLogger should be first thing we do, we then need to change lo We set it up, by some *.properties , or will we set it programatically? If by *.properties, where we should store it? Line 44: Line 45: List<String> moduleOthers = (List<String>) argMap.get(ParametersParser.OTHER); Line 46: List<Throwable> errors = (List<Throwable>) argMap.get(ParametersParser.ERROR); Line 47: if(argMap.containsKey("help") || (moduleOthers.size() > 0 && moduleOthers.get(0).equals("help"))) { Line 58: for(Throwable t : errors) { Line 59: if(t instanceof ExitException) { Line 60: logger.error(t.getMessage()); Line 61: throw t; Line 62: } > why do you throw only exit exception, best to debug/error all and just exit Done Line 63: } Line 64: } Line 65: Line 66: if (moduleOthers.size() < 1) { Line 71: String module = moduleOthers.get(0); Line 72: ModuleService moduleService = moduleServices.get(module); Line 73: if (moduleService == null) { Line 74: logger.error("No such '{}' module exists.", module); Line 75: logger.info(getModules(moduleServices)); > no need to list modules if error, user should run --help or similar to acqu Done Line 76: throw new ExitException(1); Line 77: } Line 78: moduleService.parseArguments(moduleOthers); Line 79: loadExtensions(moduleService, argMap); Line 85: String message = t.getMessage() != null ? t.getMessage() : t.getClass().getName(); Line 86: logger.error(message); Line 87: logger.debug(message, t); Line 88: } Line 89: logger.debug("Exitting with status '{}'", exitStatus); > Exiting? Done Line 90: System.exit(exitStatus); Line 91: } Line 92: Line 93: private static void loadExtensions(ModuleService moduleService, Map<String, Object> argMap) { Line 111: entry.setValue(extensionsManager.getExtensionByName(entry.getKey())); Line 112: } Line 113: } Line 114: Line 115: private static Map<String, ModuleService> loadModules(Class cls) { > please debug modules that are available. Done Line 116: Map<String, ModuleService> modules = new HashMap<>(); Line 117: if(cls != null) { Line 118: Map<String, ExtensionProxy> proxies = new HashMap<>(); Line 119: ExtMap context = new ExtMap() Line 160: private static String getModules(Map<String, ModuleService> modules) { Line 161: StringBuilder sb = new StringBuilder("Available Modules:"); Line 162: for(ModuleService entry : modules.values()) { Line 163: sb.append( Line 164: String.format("%n %-10s - %s", entry.getName(), entry.getDescription()) > why do you new line at beginning of line? multi line text should also end w Done Line 165: ); Line 166: } Line 167: return sb.toString(); Line 168: } https://gerrit.ovirt.org/#/c/37886/19/backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/logger/LoggerServiceImpl.java File backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/logger/LoggerServiceImpl.java: Line 95: if(t instanceof ExitException) { Line 96: logger.error(t.getMessage()); Line 97: throw (ExitException)t; Line 98: } Line 99: } > same, we can throw exit exception but error/debug all. Done Line 100: } Line 101: } Line 102: Line 103: @Override https://gerrit.ovirt.org/#/c/37886/19/backend/manager/modules/uutils/src/main/java/org/ovirt/engine/core/uutils/cli/parser/ExitException.java File backend/manager/modules/uutils/src/main/java/org/ovirt/engine/core/uutils/cli/parser/ExitException.java: > please move exit exception to the tool, the cli should not use it now. Done Line 1: package org.ovirt.engine.core.uutils.cli.parser; Line 2: Line 3: public class ExitException extends RuntimeException { Line 4: private int exitCode; https://gerrit.ovirt.org/#/c/37886/19/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 30: /** Line 31: * Key under which are stored exceptions during parsing in map <code>arguments</code>. Line 32: * Use it to obtain exception which happend during parsing. Line 33: */ Line 34: public static final String ERROR = "__error__"; > PARAMETERS_KEY_ERRORS? Done Line 35: Line 36: private static final String LONG_PREFIX = "--"; Line 37: private static final Properties defaultProperties = loadProperties(ParametersParser.class, "defaults.properties"); Line 38: Line 47: optional_argument, Line 48: no_argument, Line 49: } Line 50: Line 51: public ParametersParser(InputStream resource, String prefix) { > Sorry for confusion, I can't since perfix is different each time. can I get sure, I'll add it Line 52: this.properties = loadProperties(resource); Line 53: this.prefix = prefix; Line 54: parseProperties(); Line 55: } Line 53: this.prefix = prefix; Line 54: parseProperties(); Line 55: } Line 56: Line 57: public Map<String, Object> parse(String[] args) { > String... args? Done Line 58: return parse(new ArrayList<String>(Arrays.asList(args))); Line 59: } Line 60: Line 61: public Map<String, Object> parse(List<String> args) { Line 80: Line 81: ParserArgument parserArgument = arguments.get(key); Line 82: if(parserArgument == null) { Line 83: errors.add( Line 84: new ExitException( > this can be invalid argument exception. Done Line 85: String.format("Invalid argument '%1$s'", arg), Line 86: 1 Line 87: ) Line 88: ); Line 100: } Line 101: } Line 102: if(parserArgument.getType() == ArgumentType.has_argument && value == null) { Line 103: errors.add( Line 104: new ExitException( > same Done Line 105: String.format("Value is required, but missing for argument '%1$s'", key), Line 106: 1 Line 107: ) Line 108: ); Line 255: for(String arg : getUsageArguments()) { Line 256: ParserArgument argument = this.arguments.get(arg); Line 257: help.append( Line 258: String.format( Line 259: "%n --%-30s%-60s", > please always put new line at end of line. Done Line 260: arg + (argument.getType() != ArgumentType.no_argument ? "=[" + argument.getMetavar() + "]" : ""), Line 261: argument.getHelp() Line 262: ) Line 263: ); Line 291: } Line 292: } Line 293: Line 294: private static Properties loadProperties(Class<?> clazz, String resource) { Line 295: return loadProperties(clazz.getResourceAsStream(resource)); > try with resources for the InputStream? Done Line 296: } -- 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: 19 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: mooli tayer <mta...@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
