Martin Peřina has posted comments on this change. Change subject: tools: Replaces log4j with JUL as backend for manage-domains ......................................................................
Patch Set 3: (6 comments) http://gerrit.ovirt.org/#/c/32853/3/backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/tools/ManageDomainsExecutor.java File backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/tools/ManageDomainsExecutor.java: Line 16: */ Line 17: public class ManageDomainsExecutor { Line 18: public static void setupDefaultLogging() { Line 19: try (InputStream is = ManageDomainsExecutor.class.getResourceAsStream( Line 20: "/engine-manage-domains/logging.properties")) { > I think it can be relative, so put the properties file in same package and It's the same concept as we currently use for loading log4j.xml. Is there any reason why to hide properties file in the same package as class that loads it? Line 21: JavaLoggingUtils.setupLogging(is); Line 22: } catch (IOException ex) { Line 23: throw new IllegalArgumentException( Line 24: String.format("Error loading default loggging configuration from: %s", ex.getMessage()), Line 28: Line 29: public static void setupCustomLogging(String configFile, String logFile, String logLevel) { Line 30: if (configFile != null) { Line 31: try (InputStream is = new FileInputStream(configFile)) { Line 32: JavaLoggingUtils.setupLogging(is); > there is no reason to do this here and not in the properties file. Sorry, but I don't understand what do you mean. Line 33: } catch (IOException ex) { Line 34: throw new IllegalArgumentException( Line 35: String.format("Error loading loggging configuration from '%s': %s", configFile, ex.getMessage()), Line 36: ex); http://gerrit.ovirt.org/#/c/32853/3/backend/manager/modules/builtin-extensions/src/main/resources/engine-manage-domains/logging.properties File backend/manager/modules/builtin-extensions/src/main/resources/engine-manage-domains/logging.properties: Line 6: # To change logging properties for engine-manage-domains user command line Line 7: # options --log-file, --log-level and --log-config Line 8: Line 9: # Set root level to ALL, so it can be changed per handler using command line parameter --log-level Line 10: .level=ALL > why not default to ERROR or even OFF? default of debug is strange. It has to be set to ALL, if it's set to OFF, then nothing is logged even when handler level is set to DEBUG. Line 11: Line 12: # Default format of log line is: DATE TIME LEVEL [SOURCE] Message Stacktrace Line 9: # Set root level to ALL, so it can be changed per handler using command line parameter --log-level Line 10: .level=ALL Line 11: Line 12: # Default format of log line is: DATE TIME LEVEL [SOURCE] Message Stacktrace Line 13: java.util.logging.SimpleFormatter.format=%1$tY-%1$tm-%1$td %1$tH:%1$tM:%1$tS,%1$tL %4$-5s [%2$s] %5$s%6$s%n > why don't you use your own java formatter? or better... if you use the simp The syntax is the same as in SimpleFormatter, JavaLoggingFormatter just convert level names to the current syntax, so level names are the same for all engine logs. http://gerrit.ovirt.org/#/c/32853/3/backend/manager/modules/utils/pom.xml File backend/manager/modules/utils/pom.xml: Line 103 Line 104 Line 105 Line 106 Line 107 > when do we remove this? Not sure what we need this for, I will try to remove it and run all unit tests ... Line 138 Line 139 Line 140 Line 141 Line 142 > when do we remove this? I'm going to remove all log4j dependencies when log4j is replaced with slf4j in all project classes. -- To view, visit http://gerrit.ovirt.org/32853 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I16afc336c26615978392f7d4ff911bf646c42df6 Gerrit-PatchSet: 3 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Peřina <mper...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Martin Peřina <mper...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com> Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.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
