Alon Bar-Lev has posted comments on this change. Change subject: aaa: extensions test tool: logger ......................................................................
Patch Set 2: (23 comments) http://gerrit.ovirt.org/#/c/37886/2/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 4: import java.util.Map; Line 5: Line 6: public class ExtensionsToolExecutor { Line 7: Line 8: public static void main(String[] args) { in main you should setup log level and file [at least] before. Line 9: Map<String, String> toolArguments = null; Line 10: StringBuilder sb = new StringBuilder(); Line 11: try { Line 12: List<ModuleService> moduleServices = ExtensionsToolLoader.load(ModuleService.class); Line 8: public static void main(String[] args) { Line 9: Map<String, String> toolArguments = null; Line 10: StringBuilder sb = new StringBuilder(); Line 11: try { Line 12: List<ModuleService> moduleServices = ExtensionsToolLoader.load(ModuleService.class); please load all modules and put them nicely into a map. then you can process this map instead of ugly code. Line 13: for(ModuleService moduleService : moduleServices) { Line 14: sb.append(moduleService.getName() + " "); Line 15: if ((args.length < 1) || !moduleService.getName().equals(args[0])) { Line 16: continue; Line 14: sb.append(moduleService.getName() + " "); Line 15: if ((args.length < 1) || !moduleService.getName().equals(args[0])) { Line 16: continue; Line 17: } Line 18: toolArguments = moduleService.parseArguments(args); if all ok, then we need to load extensions here... the extensions directory or module are global parameters. Line 19: moduleService.run(toolArguments); Line 20: } Line 21: Line 22: if(toolArguments == null) { http://gerrit.ovirt.org/#/c/37886/2/backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/core/ExtensionsToolLoader.java File backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/core/ExtensionsToolLoader.java: Line 14: services.add(module); Line 15: } Line 16: } Line 17: return services; Line 18: } this can be simple method within the previous class, unless you think it should be reused. http://gerrit.ovirt.org/#/c/37886/2/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 2: Line 3: import java.io.*; Line 4: import java.util.Map; Line 5: Line 6: public abstract class ModuleService { please use only interfaces for stuff we provide within service. it should be pure abstract. Line 7: Line 8: public static final String ARG_LOG_LEVEL = "--log-level"; Line 9: public static final String ARG_LOG_FILE = "--log-file"; Line 10: public static final String ARG_EXT_CONFIG = "--extensions-config"; Line 6: public abstract class ModuleService { Line 7: Line 8: public static final String ARG_LOG_LEVEL = "--log-level"; Line 9: public static final String ARG_LOG_FILE = "--log-file"; Line 10: public static final String ARG_EXT_CONFIG = "--extensions-config"; these are not per module but per core. Line 11: private static final String HELP = "help"; Line 12: Line 13: public String getUsage(Class module) throws Exception { Line 14: BufferedReader reader = new BufferedReader(new InputStreamReader(module.getResourceAsStream(HELP))); Line 7: Line 8: public static final String ARG_LOG_LEVEL = "--log-level"; Line 9: public static final String ARG_LOG_FILE = "--log-file"; Line 10: public static final String ARG_EXT_CONFIG = "--extensions-config"; Line 11: private static final String HELP = "help"; I do not think help should be valid. Line 12: Line 13: public String getUsage(Class module) throws Exception { Line 14: BufferedReader reader = new BufferedReader(new InputStreamReader(module.getResourceAsStream(HELP))); Line 15: StringBuilder out = new StringBuilder(); Line 18: out.append(line); Line 19: out.append(System.getProperty("line.separator")); Line 20: } Line 21: reader.close(); Line 22: return out.toString(); you can just read entire content and convert to UTF-8, no reason to do the line magic. worse case you replace all \n with line.separator before print to console, but I do not think it is required, as java System.print("XXX\n") works on all platforms. Line 23: } Line 24: Line 25: public void printHelp(Class module) { Line 26: try { Line 19: out.append(System.getProperty("line.separator")); Line 20: } Line 21: reader.close(); Line 22: return out.toString(); Line 23: } this should be in utility class not in base Line 24: Line 25: public void printHelp(Class module) { Line 26: try { Line 27: System.out.println(getUsage(module)); Line 28: } catch (Exception ex) { Line 29: ex.printStackTrace(); Line 30: System.err.println("Error reading help content."); Line 31: } Line 32: } not sure what this is. Line 33: Line 34: public abstract Map<String, String> parseArguments(String[] args) throws Exception; Line 35: Line 36: public abstract void run(Map<String, String> args) throws Exception; Line 34: public abstract Map<String, String> parseArguments(String[] args) throws Exception; Line 35: Line 36: public abstract void run(Map<String, String> args) throws Exception; Line 37: Line 38: public abstract String getName(); these I understand :) Line 39: http://gerrit.ovirt.org/#/c/37886/2/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 24: public static final String LOG_RECORD = "log-record"; Line 25: public static final String ARG_ACTION_NAME = "--logger-name"; Line 26: public static final String ARG_ACTION_LEVEL = "--level"; Line 27: public static final String ARG_ACTION_MSG = "--message"; Line 28: public static final String ACTION_HELP = "--help"; one time used strings can be just specified, there is no need for constants. Line 29: Line 30: public LoggerServiceImpl() { Line 31: } Line 32: Line 47: if(argMap.containsKey(ACTION_HELP)) { Line 48: action = ACTION_HELP; Line 49: } Line 50: Line 51: return argMap; you can store this as member, no reason to return. Line 52: } Line 53: Line 54: @Override Line 55: public void run(Map<String, String> args) throws Exception { Line 60: Line 61: File loggerConfig = new File(args.<String> get(ARG_EXT_CONFIG)); Line 62: if (!loggerConfig.exists()) { Line 63: throw new Exception("Could not find logger extension configuration at the specified path"); Line 64: } this should be loaded by core and be ready. you should have already constructed and initialized extensionmanager instance passed into the service. Line 65: ExtensionsManager extensionsManager = new ExtensionsManager(); Line 66: ExtensionProxy extensionProxy = extensionsManager.initialize(extensionsManager.load(loggerConfig)); Line 67: LogRecord logRecord = new LogRecord( Line 68: Level.parse(args.get(ARG_ACTION_LEVEL)), Line 63: throw new Exception("Could not find logger extension configuration at the specified path"); Line 64: } Line 65: ExtensionsManager extensionsManager = new ExtensionsManager(); Line 66: ExtensionProxy extensionProxy = extensionsManager.initialize(extensionsManager.load(loggerConfig)); Line 67: LogRecord logRecord = new LogRecord( I think we should also add log name parameter, maybe others. Line 68: Level.parse(args.get(ARG_ACTION_LEVEL)), Line 69: args.get(ARG_ACTION_MSG) Line 70: ); Line 71: ExtMap output = extensionProxy.invoke( Line 75: ).mput( Line 76: Logger.InvokeKeys.LOG_RECORD, Line 77: logRecord Line 78: ) Line 79: ); please flush and close as well, oh! you are doing something strange.... the sequence should be static.... publish flush close so per single invocation we do all. Line 80: } Line 81: Line 82: private ExtendedCliParser initParser(String action) { Line 83: ExtendedCliParser parser = new ExtendedCliParser(); Line 122: } Line 123: Line 124: @Override Line 125: public String getName() { Line 126: return MODULE; not sure why this constant is good... :) just put the string. Line 127: } Line 128: Line 129: private ExtUUID getCommand(String command) { Line 130: if(command == null) { Line 123: Line 124: @Override Line 125: public String getName() { Line 126: return MODULE; Line 127: } please put trivials like these first in interface. Line 128: Line 129: private ExtUUID getCommand(String command) { Line 130: if(command == null) { Line 131: return Logger.InvokeCommands.PUBLISH; http://gerrit.ovirt.org/#/c/37886/2/backend/manager/extension-tool/src/main/resources/org/ovirt/engine/exttool/logger/services/help File backend/manager/extension-tool/src/main/resources/org/ovirt/engine/exttool/logger/services/help: put .txt suffix so we know what it is when running zipinfo Line 1: ovirt-engine-extension-tool Line 2: --log-level=INFO Line 3: --log-file= Line 4: --extensions-config=<location of extensions configuration> Line 8: FLUSH Line 9: CLOSE Line 10: --logger-name="org.ovirt.engine" Line 11: --level=INFO Line 12: --message="test" please always put new lines in sources http://gerrit.ovirt.org/#/c/37886/2/ovirt-engine.spec.in File ovirt-engine.spec.in: Line 660: %{engine_jboss_modules}/common/org/ovirt/engine/core/tools/main/tools.jar Line 661: %{engine_jboss_modules}/common/org/ovirt/engine/core/utils/main/utils.jar Line 662: %{engine_jboss_modules}/common/org/ovirt/engine/core/uutils/main/uutils.jar Line 663: %{engine_jboss_modules}/common/org/ovirt/engine/extensions/builtin/main/builtin.jar Line 664: %{engine_jboss_modules}/org/ovirt/engine/core/extension-tool/main/extension-tool.jar hmmm.... it should be in common I think. Line 665: __EOF__ Line 666: Line 667: # Needed for compatibility if package is different than the directory structure Line 668: %if "%{name}" != "%{engine_name}" http://gerrit.ovirt.org/#/c/37886/2/packaging/bin/ovirt-engine-extensions-tool.sh File packaging/bin/ovirt-engine-extensions-tool.sh: Line 20: Line 21: exec "${JAVA_HOME}/bin/java" \ Line 22: -Djboss.modules.write-indexes=false \ Line 23: -Djava.security.auth.login.config="${ENGINE_USR}/conf/jaas.conf" \ Line 24: -Djava.util.logging.config.file="${ENGINE_USR}/conf/extensions-tool/logging.conf" \ -Djava.util.logging.config.file="${OVIRT_LOGGING_PROPERTIES}" \ Line 25: -jar "${JBOSS_HOME}/jboss-modules.jar" \ Line 26: -dependencies org.ovirt.engine.core.extension-tool \ Line 27: -cp "${CLASSPATH}:${ENGINE_USR}/conf/extensions-tool/commons-logging.properties" \ Line 28: -class org.ovirt.engine.exttool.core.ExtensionsToolExecutor \ Line 23: -Djava.security.auth.login.config="${ENGINE_USR}/conf/jaas.conf" \ Line 24: -Djava.util.logging.config.file="${ENGINE_USR}/conf/extensions-tool/logging.conf" \ Line 25: -jar "${JBOSS_HOME}/jboss-modules.jar" \ Line 26: -dependencies org.ovirt.engine.core.extension-tool \ Line 27: -cp "${CLASSPATH}:${ENGINE_USR}/conf/extensions-tool/commons-logging.properties" \ why do we need commons-logging? Line 28: -class org.ovirt.engine.exttool.core.ExtensionsToolExecutor \ -- To view, visit http://gerrit.ovirt.org/37886 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie06113c5d56a49e58d557c851f9ff00b9a9ca409 Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ondřej Macháček <[email protected]> Gerrit-Reviewer: Alon Bar-Lev <[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
