Martin Peřina has posted comments on this change. Change subject: core: Add common methods for runtime log4j setup ......................................................................
Patch Set 1: (5 comments) http://gerrit.ovirt.org/#/c/26588/1/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/log/Log4jUtils.java File backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/log/Log4jUtils.java: Line 28: */ Line 29: public static void setupLogging(String log4jConfig, URL defaultUrl) { Line 30: try { Line 31: URL url = defaultUrl; Line 32: if (!StringUtils.isBlank(log4jConfig)) { > isEmpty? I don't know anybody who would want to use only whitespace characters for file/directory :-) But OK, I will test only if null Line 33: File f = new File(log4jConfig); Line 34: if (f.exists()) { Line 35: url = f.toURI().toURL(); Line 36: } else { Line 34: if (f.exists()) { Line 35: url = f.toURI().toURL(); Line 36: } else { Line 37: url = new URL(log4jConfig); Line 38: } > are you sure this is valid check? can't you check if it is URL based on pat It's because of DomConfigurator(URL) method: DomConfigurator.configure(new URL("file:///tmp/log.txt")); // OK DomConfigurator.configure(new URL("/tmp/log.txt")); // ERROR If I didn't use this test, all filenames in --lo4j-config will have to start with "file://" prefix, which IMO is not user friendly Line 39: } Line 40: LogManager.resetConfiguration(); Line 41: DOMConfigurator.configure(url); Line 42: } catch (IOException | FactoryConfigurationError ex) { Line 55: public static void setupFileAppender(String fileName, String levelName) { Line 56: if (StringUtils.isNotBlank(fileName)) { Line 57: Level level; Line 58: if (StringUtils.isBlank(levelName)) { Line 59: level = Level.DEBUG; > default should be info Well, in current log4j.xml the DEBUG level is set, but I can change to INFO Line 60: } else { Line 61: level = Level.toLevel(levelName, null); Line 62: if (level == null) { Line 63: throw new IllegalArgumentException(String.format("Invalid log level value: '%s'", levelName)); Line 67: FileAppender fa = null; Line 68: try { Line 69: fa = new FileAppender(new PatternLayout("%d %-5p [%c] %m%n"), fileName, true); Line 70: fa.setThreshold(level); Line 71: } catch (SecurityException | IOException ex) { > I think this try/catch should wrap entire function not this place, as there Well, the exception can be thrown only in this call. Also if entire function is in try block, I wouldn't be able to distinguish between invalid level and file access error. Line 72: throw new IllegalArgumentException( Line 73: String.format("Error accessing log file '%s': '%s'", fileName, ex.getMessage()), Line 74: ex); Line 75: } Line 77: Logger rootLogger = Logger.getRootLogger(); Line 78: if (rootLogger.getLevel() == Level.OFF) { Line 79: // by default it's set to OFF, so we need to enabled it before adding appender Line 80: rootLogger.setLevel(Level.ALL); Line 81: } > ok, I understand... this is for you to have isDebugEnabled() set by log le By default I had to set root logger level to OFF, otherwise "log4j No appenders ..."warning appears in stderr when Logger is accessed in application. Please look at my reply to your comment in patch 26591 Line 82: rootLogger.addAppender(fa); Line 83: } Line 84: } -- To view, visit http://gerrit.ovirt.org/26588 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic00ea0af643e8c978fd67eb8c56e2329d43e776d Gerrit-PatchSet: 1 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: Eli Mesika <emes...@redhat.com> Gerrit-Reviewer: Martin Peřina <mper...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@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
