Ondřej Macháček has posted comments on this change. Change subject: extensions test tool: logger ......................................................................
Patch Set 5: (26 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? Done 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 listFilesWithPropertiesSuffi Done 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. Done 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 { > no, as drivers we use java.util.logging Done 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 I have map to access services by name. It's not sorted. I'll sort it here. 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? crash.. fixed :) 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 Done 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. Done 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. Done 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 Done 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 Done 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 Here? Here I don't have any information, even if the module provided on cmd line is syntactically/semantically correct. Like you said, it should be after extension load ... not module, no? 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 This is need only or 1st parse. we shoould not do anything if there are no arguments, no? If there are we looks what's in there.. 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... Done 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 Done 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 Done 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 Done 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 Done 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 :) Done 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 :) changed.. 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 Done 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. Done 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: } > no, we should also get extension name to use for logger, I think this is in Done 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 > the benefit is allowing maintaining single properties file for multiple usa Done 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); > I finnally understand how you mean to work it. I thought it should be bit d Done 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 Done 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]) -- 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 <[email protected]> Gerrit-Reviewer: Alon Bar-Lev <[email protected]> Gerrit-Reviewer: Ondra Machacek <[email protected]> Gerrit-Reviewer: Ondřej Macháček <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
