Alon Bar-Lev 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. 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 log level based on parameters. but we need log for the initialization process... and set level based on environment at that point. 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, no? at the end, the exit exception is local to this main module. 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 acquire it. 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? 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. 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 138: if(logfile != null) { Line 139: FileHandler fh = new FileHandler(logfile); Line 140: fh.setFormatter(new SimpleFormatter()); Line 141: logger.addHandler(fh); Line 142: } aren't these already set by logging.properties? Line 143: } Line 144: Line 145: private static List<File> listFiles(String directory, String suffix) { Line 146: File dir = new File(directory); 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 with new line. 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. 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/ParametersParser.java File backend/manager/modules/uutils/src/main/java/org/ovirt/engine/core/uutils/cli/parser/ParametersParser.java: Line 25: /** Line 26: * Key under which are stored other arguments in map <code>arguments</code>. Line 27: * Use it to obtain rest of arguments, which was not parsed by parser. Line 28: */ Line 29: public static final String OTHER = "__other__"; PARAMETER_KEY_OTHER ? 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 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? 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 53: this.prefix = prefix; Line 54: parseProperties(); Line 55: } Line 56: Line 57: public Map<String, Object> parse(String[] args) { String... args? 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. 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 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. 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? 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
