Alon Bar-Lev has posted comments on this change.

Change subject: tools: Adds helpers to configure java logging in tools
......................................................................


Patch Set 7:

(4 comments)

http://gerrit.ovirt.org/#/c/32852/7/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/log/JavaLoggingUtils.java
File 
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/log/JavaLoggingUtils.java:

Line 39:      */
Line 40:     public static void addFileHandler(String fileName) {
Line 41:         try {
Line 42:             FileHandler fh = new FileHandler(fileName);
Line 43:             fh.setFormatter(new SimpleFormatter());
I think that the formatter can be gotten out of the logging properties so you 
do not need to declare it here.
Line 44:             getOvirtLogger().addHandler(fh);
Line 45:         } catch (SecurityException | IOException ex) {
Line 46:             throw new IllegalArgumentException(
Line 47:                     String.format("Error accessing log file '%s': 
'%s'", fileName, ex.getMessage()),


Line 40:     public static void addFileHandler(String fileName) {
Line 41:         try {
Line 42:             FileHandler fh = new FileHandler(fileName);
Line 43:             fh.setFormatter(new SimpleFormatter());
Line 44:             getOvirtLogger().addHandler(fh);
don't you need to set the level of the formatter(?) to FINEST?
Line 45:         } catch (SecurityException | IOException ex) {
Line 46:             throw new IllegalArgumentException(
Line 47:                     String.format("Error accessing log file '%s': 
'%s'", fileName, ex.getMessage()),
Line 48:                     ex);


Line 63:      * Returns instance of ovirt logger (it creates new instance if 
current one is {@code null}).
Line 64:      */
Line 65:     private static Logger getOvirtLogger() {
Line 66:         if (ovirtLogger == null) {
Line 67:             ovirtLogger = Logger.getLogger("org.ovirt");
I am sorry I still do not understand why we cannot get this at static 
context... I probably missing something.

regardless, for this to work you need volatile or something.
Line 68:         }
Line 69:         return ovirtLogger;
Line 70:     }


http://gerrit.ovirt.org/#/c/32852/7/packaging/conf/tools-logging.properties
File packaging/conf/tools-logging.properties:

Line 10: # Default level for org.ovirt logger
Line 11: org.ovirt.level=INFO
Line 12: 
Line 13: # Default format of log line is: DATE TIME LEVEL [SOURCE] Message 
Stacktrace
Line 14: java.util.logging.SimpleFormatter.format=%1$tY-%1$tm-%1$td 
%1$tH:%1$tM:%1$tS,%1$tL %4$-7s [%2$s] %5$s%6$s%n
don't we want console logger? or messages we print are not via logger?


-- 
To view, visit http://gerrit.ovirt.org/32852
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie1c46ca552888b347a4ff810003e0fcc818f832f
Gerrit-PatchSet: 7
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

Reply via email to