Alon Bar-Lev has posted comments on this change. Change subject: extensions test tool: logger ......................................................................
Patch Set 5: (28 comments) https://gerrit.ovirt.org/#/c/37886/5/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 21: return prop; Line 22: } catch (IOException e) { Line 23: throw new IOException( Line 24: String.format("An error occurred while reading properties file: '%1$s'", resource) Line 25: ); the io exception you get should be good enough, no? anyway, please always include the cause exception when rethrowing Line 26: } Line 27: } Line 28: Line 29: public static List<File> listFiles(String directory) { Line 25: ); Line 26: } Line 27: } Line 28: Line 29: public static List<File> listFiles(String directory) { I suggest to add suffix parameter or call this listFilesWithPropertiesSuffix :) Line 30: File dir = new File(directory); Line 31: if (!dir.exists()) { Line 32: throw new IllegalArgumentException( Line 33: String.format("Directory %1$s doesn't exists.", directory) Line 30: File dir = new File(directory); Line 31: if (!dir.exists()) { Line 32: throw new IllegalArgumentException( Line 33: String.format("Directory %1$s doesn't exists.", directory) Line 34: ); this is valid, so no files.... worse case you can log a warning. Line 35: } Line 36: List<File> propFiles = new ArrayList<>(); Line 37: File[] dirFiles = dir.listFiles(); Line 38: if (dirFiles != null) { https://gerrit.ovirt.org/#/c/37886/5/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 12: Line 13: import java.io.File; Line 14: import java.util.*; Line 15: Line 16: public class ExtensionsToolExecutor { we still need to manage the --log-level and --log-file, and set logger to be at console and such, no? Line 17: Line 18: private static final String ARG_HELP = "help"; Line 19: Line 20: private final Logger logger = LoggerFactory.getLogger(ExtensionsToolExecutor.class); Line 24: Map<String, Object> argMap = parseArgs(args); Line 25: Map<String, ModuleService> moduleServices = load(ModuleService.class); Line 26: if (argMap.containsKey(ARG_HELP)) { Line 27: System.out.println("core help"); // TODO: help printer Line 28: System.out.println("Modules: " + StringUtils.join(moduleServices.keySet(), ", ")); please sort oh... you return treemap, better was to sort only for presentation. Line 29: return; Line 30: } Line 31: Line 32: List<String> others = (List<String>) argMap.get("__other__"); Line 29: return; Line 30: } Line 31: Line 32: List<String> others = (List<String>) argMap.get("__other__"); Line 33: String module = others.remove(0); what happens if nothing? Line 34: ModuleService moduleService = moduleServices.get(module); Line 35: if (moduleService == null) { Line 36: throw new IllegalArgumentException( Line 37: String.format("No such '%1$s' module exists. To list existing modules use --help argument.", module) Line 42: System.out.println(module + " help"); // TODO: help printer Line 43: return; Line 44: } Line 45: others = (List<String>)moduleArgMap.get("__other__"); Line 46: if(others.size() > 1) { this is for module to decide Line 47: throw new IllegalArgumentException( Line 48: String.format( Line 49: "You've provided illegal arguments '%1$s' for module '%2$s' ", StringUtils.join(others, ", "), module Line 50: ) Line 58: for(ExtensionProxy proxy : extensionsManager.getLoadedExtensions()) { Line 59: extensionsManager.initialize(proxy.getContext().<String>get(Base.ContextKeys.INSTANCE_NAME)); Line 60: } Line 61: Line 62: ExtMap context = new ExtMap().mput(Base.GlobalContextKeys.EXTENSIONS, extensionsManager.getLoadedExtensions()); why do you re-use base key for this? you can use your own key. Line 63: moduleService.setContext(context); Line 64: moduleService.run(); Line 65: } catch (IllegalArgumentException ex) { Line 66: System.out.println(ex.getMessage()); Line 59: extensionsManager.initialize(proxy.getContext().<String>get(Base.ContextKeys.INSTANCE_NAME)); Line 60: } Line 61: Line 62: ExtMap context = new ExtMap().mput(Base.GlobalContextKeys.EXTENSIONS, extensionsManager.getLoadedExtensions()); Line 63: moduleService.setContext(context); the context should be set as early as you load all modules. Line 64: moduleService.run(); Line 65: } catch (IllegalArgumentException ex) { Line 66: System.out.println(ex.getMessage()); Line 67: } catch (Throwable t) { Line 62: ExtMap context = new ExtMap().mput(Base.GlobalContextKeys.EXTENSIONS, extensionsManager.getLoadedExtensions()); Line 63: moduleService.setContext(context); Line 64: moduleService.run(); Line 65: } catch (IllegalArgumentException ex) { Line 66: System.out.println(ex.getMessage()); please always use the log, do not print directly Line 67: } catch (Throwable t) { Line 68: t.printStackTrace(); Line 69: } Line 70: } Line 64: moduleService.run(); Line 65: } catch (IllegalArgumentException ex) { Line 66: System.out.println(ex.getMessage()); Line 67: } catch (Throwable t) { Line 68: t.printStackTrace(); only in debug mode via log Line 69: } Line 70: } Line 71: Line 72: private static Map<String, ModuleService> load(Class cls) { Line 73: Map<String, ModuleService> modules = new HashMap<>(); Line 74: if(cls != null) { Line 75: ServiceLoader<ModuleService> loader = ServiceLoader.load(cls); Line 76: for (ModuleService module : loader) { Line 77: modules.put(module.getName(), module); and set context Line 78: } Line 79: } Line 80: Line 81: return modules; Line 83: Line 84: private static Map<String, Object> parseArgs(String[] args) throws Exception { Line 85: if(args.length < 1) { Line 86: throw new IllegalArgumentException("Please provide module. To list existing modules use --help argument."); Line 87: } this should be done by caller I think as this is required only for the 2nd parse. Line 88: Line 89: Properties prop = CoreUtil.loadProperties(ExtensionsToolExecutor.class); Line 90: ParametersParser parser = new ParametersParser(prop); Line 91: Map<String, Object> argMap = parser.parse(args); Line 89: Properties prop = CoreUtil.loadProperties(ExtensionsToolExecutor.class); Line 90: ParametersParser parser = new ParametersParser(prop); Line 91: Map<String, Object> argMap = parser.parse(args); Line 92: Line 93: return argMap; no need for temp vars... return new ParametersParser( CoreUtil.loadProperties(ExtensionsToolExecutor.class) ).parse(args); Line 94: } https://gerrit.ovirt.org/#/c/37886/5/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 7: import java.util.Map; Line 8: Line 9: public interface ModuleService { Line 10: Line 11: public Map<String, Object> parseArguments(List<String> args) throws Exception; third Line 12: Line 13: public void run() throws Exception; Line 14: Line 15: public void setContext(ExtMap context); Line 9: public interface ModuleService { Line 10: Line 11: public Map<String, Object> parseArguments(List<String> args) throws Exception; Line 12: Line 13: public void run() throws Exception; forth Line 14: Line 15: public void setContext(ExtMap context); Line 16: Line 17: public String getName(); Line 11: public Map<String, Object> parseArguments(List<String> args) throws Exception; Line 12: Line 13: public void run() throws Exception; Line 14: Line 15: public void setContext(ExtMap context); second Line 16: Line 17: public String getName(); Line 13: public void run() throws Exception; Line 14: Line 15: public void setContext(ExtMap context); Line 16: Line 17: public String getName(); first https://gerrit.ovirt.org/#/c/37886/5/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 16: Line 17: public class LoggerServiceImpl implements ModuleService { Line 18: Line 19: private ExtMap context; Line 20: private Map<String, Object> argMap; args will be nicer :) Line 21: private static Map<String, Class<? extends Runnable>> actions = new HashMap<>(); Line 22: static { Line 23: actions.put( Line 24: "log-record", Line 22: static { Line 23: actions.put( Line 24: "log-record", Line 25: ModuleLogRecord.class Line 26: ); I thought to have a method per action, but ok :) Line 27: } Line 28: Line 29: public LoggerServiceImpl() { Line 30: argMap = new HashMap<>(); Line 46: CoreUtil.loadProperties(getClass()) Line 47: ); Line 48: argMap = parser.parse(args); Line 49: Line 50: return argMap; no need for temp vars please decide if you want to return or store the args Line 51: } Line 52: Line 53: @Override Line 54: public void run() throws Exception { Line 59: String.format("No such action '%1$s' exists for module '%2$s'", action, getName()) Line 60: ); Line 61: } Line 62: Line 63: actions.get(action).getDeclaredConstructor(getClass()).newInstance(this).run(); you can keep a reference to instance no need to do dynamic here. or you can do this by anonymous class as I showed. in either case you have instance Line 64: } Line 65: Line 66: class ModuleLogRecord implements Runnable { Line 67: Line 77: List<ExtensionProxy> proxies = ((List<ExtensionProxy>) context.get(Base.GlobalContextKeys.EXTENSIONS)); Line 78: for(ExtensionProxy proxy : proxies) { Line 79: if(!proxy.getContext().<String>get(Base.ContextKeys.INSTANCE_NAME).equals(loggerName)) { Line 80: continue; Line 81: } not sure I understand... you should find your extension based on name, you can acquire the extension out of the extensions manager. the logger name is something else, it is attribute of the pulish call. Line 82: proxy.invoke( Line 83: new ExtMap().mput( Line 84: Base.InvokeKeys.COMMAND, Line 85: Logger.InvokeCommands.PUBLISH https://gerrit.ovirt.org/#/c/37886/5/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 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 Line 12: arg.help.name = help Line 13: arg.help.help = show possible modules you can use please add prefix, somekind of application prefix so that same property file can contain several usages. https://gerrit.ovirt.org/#/c/37886/5/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 63: if(arg.equals(LONG_PREFIX)) { Line 64: break; Line 65: } Line 66: if(!arg.startsWith(LONG_PREFIX)) { Line 67: others.add(arg); then you switch state, all remaining tokens should go into others, so you actually need to break the loop. Line 68: } else { Line 69: String[] argVal = parseArgument(arg.substring(2)); Line 70: ParserArgument parserArgument = arguments.get(argVal[0]); Line 71: if(parserArgument == null) { Line 69: String[] argVal = parseArgument(arg.substring(2)); Line 70: ParserArgument parserArgument = arguments.get(argVal[0]); Line 71: if(parserArgument == null) { Line 72: others.add(arg); Line 73: continue; this is syntax error Line 74: } Line 75: if(parserArgument.getType() == ArgumentType.required_argument && argVal[1] == null) { Line 76: throw new IllegalArgumentException( Line 77: String.format("Argument's '%1$s' value is required", argVal[0]) Line 137: ); Line 138: String pattern = getArgAttrValue(properties, name, "matcher"); Line 139: argument.setMatcher( Line 140: Pattern.compile( Line 141: pattern != null ? pattern : ".*" defaults should be in properties file. so you actually load: default.properties whatever.properties anything that was not specified in whatever is set by default you can have: xxx.args.__default__.xxx to fallback default. in anyway... you have in properties: p.getProperty("key", "default") but I do like the default properties. but even if you do not have default, you can put the default when constructing the ParseArgument as default, if not overridden then this what remains. Line 142: ) Line 143: ); Line 144: String convert = getArgAttrValue(properties, name, "convert"); Line 145: try { Line 166: arguments.put(name, argument); Line 167: } Line 168: } Line 169: Line 170: private Set<String> getUsageArguments(Properties properties) { you can as well return a list of String, String and avoid the 2nd parsing in parseArgument or any other solution to avoid multiple going over here... for example array of maps in which you have key and value. Line 171: SortedSet<String> args = new TreeSet<>(new Comparator<String>() { Line 172: @Override Line 173: public int compare(String o, String t1) { Line 174: return o.compareTo(t1); -- 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: 5 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
