Alon Bar-Lev has posted comments on this change. Change subject: extensions test tool: logger ......................................................................
Patch Set 7: (13 comments) https://gerrit.ovirt.org/#/c/37886/7/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 8: public class CoreUtil { Line 9: Line 10: public static Properties loadProperties(Class<?> clazz, String resource) { Line 11: try ( Line 12: Reader is = new InputStreamReader(clazz.getResourceAsStream(resource), "UTF-8") Charset.forName("UTF-8") to avoid UnsupportedEncodingException Line 13: ) { Line 14: Properties prop = new Properties(); Line 15: prop.load(is); Line 16: return prop; https://gerrit.ovirt.org/#/c/37886/7/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 37: if (argMap.containsKey(ARG_HELP)) { 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: System.exit(0); we should really avoid exit at middle of program... better use conditionals and/or divide to smaller functions. Line 42: } Line 43: if(others.isEmpty()) { Line 44: throw new IllegalArgumentException("Module must be provided"); Line 45: } Line 59: loadExtensions(moduleService, argMap); Line 60: moduleService.run(); Line 61: } catch (Throwable t) { Line 62: String message = t.getMessage() != null ? t.getMessage() : t.getClass().getName(); Line 63: System.err.println(message); we use console logger, right? so we actually see all log.error/log.debug in console anyway, no? Line 64: logger.error(message); Line 65: logger.debug(message, t); Line 66: System.exit(1); Line 67: } Line 73: Line 74: List<File> files; Line 75: Object extensionFile = argMap.get("extension-file"); Line 76: if(extensionFile != null) { Line 77: if(extensionFile instanceof List) { easier to mark argument as List (multi-value) or not, so you always get single type and reduce logic. Line 78: files = (List<File>) argMap.get("extension-file"); Line 79: } else { Line 80: files = new ArrayList<>(); Line 81: files.add((File)extensionFile); Line 77: if(extensionFile instanceof List) { Line 78: files = (List<File>) argMap.get("extension-file"); Line 79: } else { Line 80: files = new ArrayList<>(); Line 81: files.add((File)extensionFile); Arrays.asList() Line 82: } Line 83: } else { Line 84: files = listFiles((String)argMap.get("extensions-dir"), "properties"); Line 85: } Line 97: private static Map<String, ModuleService> loadModules(Class cls) { Line 98: Map<String, ModuleService> modules = new HashMap<>(); Line 99: if(cls != null) { Line 100: Map<String, ExtensionProxy> proxies = new HashMap<>(); Line 101: ExtMap context = new ExtMap().mput(Base.GlobalContextKeys.EXTENSIONS_MAP, proxies); now I get it... this is your own context, the key should be here as it is private. but why do you need this? all loaded extensions can reference each other using the global within base. need still to figure out why it is required in context. Line 102: Line 103: ServiceLoader<ModuleService> loader = ServiceLoader.load(cls); Line 104: for (ModuleService module : loader) { Line 105: modules.put(module.getName(), module); Line 134: private static List<File> listFiles(String directory, String suffix) { Line 135: File dir = new File(directory); Line 136: List<File> propFiles = new ArrayList<>(); Line 137: if (!dir.exists()) { Line 138: logger.warn("Directory {0} doesn't exists.", directory); {} Line 139: return propFiles; Line 140: } Line 141: File[] dirFiles = dir.listFiles(); Line 142: if (dirFiles != null) { https://gerrit.ovirt.org/#/c/37886/7/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 9: public String getName(); Line 10: Line 11: public String getDescription(); Line 12: Line 13: public boolean isHelp(); needHelp() ? doHelp()? Line 14: Line 15: public void setContext(ExtMap context); Line 16: Line 17: public ExtMap getContext(); https://gerrit.ovirt.org/#/c/37886/7/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 68: Line 69: @Override Line 70: public void run() throws Exception { Line 71: List<String> argsList = (List <String>)args.get("__other__"); Line 72: Runnable action = actions.get(argsList.remove(0)); not sure it is good to modify args that were provided, action can ignore it if it likes, removing will not allow reusing class. Line 73: if(action == null) { Line 74: throw new IllegalArgumentException( Line 75: String.format("No such action '%1$s' exists for module '%2$s'", action, getName()) Line 76: ); https://gerrit.ovirt.org/#/c/37886/7/backend/manager/extension-tool/src/main/resources/org/ovirt/engine/exttool/core/arguments.properties.in File backend/manager/extension-tool/src/main/resources/org/ovirt/engine/exttool/core/arguments.properties.in: Line 15: core.arg.log-level.convert = java.util.logging.Level Line 16: core.arg.log-level.default = INFO Line 17: core.arg.log-level.matcher = FINEST|FINER|FINE|CONFIG|INFO|WARNING|SEVERE|ALL Line 18: core.arg.help.name = help Line 19: core.arg.help.help = show help for core user does not know what core is :) https://gerrit.ovirt.org/#/c/37886/7/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 96: Matcher m = parserArgument.getMatcher().matcher(value); Line 97: if (!m.find()) { Line 98: throw new IllegalArgumentException( Line 99: String.format( Line 100: "Argument's '%1$s' value don't match pattern: ", m.pattern() something wrong with the arguments place holders. if you indent each in new line, move the m.parrern() as well :) Line 101: ) Line 102: ); Line 103: } Line 104: Line 197: String.format("Invalid option '%1$s'", argName) Line 198: ); Line 199: } Line 200: Line 201: Set<String> usageArgs = args.get(param[0]); I lost track what this function is doing, will revisit in future. Line 202: if(usageArgs == null) { Line 203: usageArgs = new TreeSet<>(new Comparator<String>() { Line 204: @Override Line 205: public int compare(String o, String t1) { Line 239: return prop; Line 240: } catch (IOException ioe) { Line 241: throw new RuntimeException(ioe); Line 242: } Line 243: } isn't this duplicate of different location? -- 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: 7 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
