Alon Bar-Lev has posted comments on this change. Change subject: core: Add common methods for runtime log4j setup ......................................................................
Patch Set 2: (3 comments) http://gerrit.ovirt.org/#/c/26588/2/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 23: * path or URL to external log4j.xml file Line 24: * @param defaultUrl Line 25: * URL with default log4j.xml Line 26: */ Line 27: public static void setupLogging(String log4jConfig, URL defaultUrl) { > String is the value set in --log-file, I just pass the responsibility to lo I know... I imply that signature should be symmetric. And there having conditional within function is not good design. This code should be: void _setupLogging(URL config) { } void setupLogging(String log4jConfig, URL defaultUrl) { _setupLogging(log4jConfig != null ? new URL(log4jConfig) : defaultUrl); } Then you should ask your-self, why do you need this function that does the conditional, and why does it get different types for same artifact. So it should be: void setupLogging(URL log4jConfig, URL defaultUrl) { _setupLogging(log4jConfig != null ? log4jConfig : defaultUrl); } And then you should ask, why do you need this function at all. Line 28: try { Line 29: LogManager.resetConfiguration(); Line 30: if (log4jConfig == null) { Line 31: DOMConfigurator.configure(defaultUrl); Line 46: * @param levelName Line 47: * log level to use Line 48: */ Line 49: public static void addFileAppender(String fileName, String levelName) { Line 50: if (fileName != null) { > I don't validate command line args for --log-file and --log-level, validati yes, but why don't you? Line 51: Level level = Level.INFO; Line 52: if (levelName != null) { Line 53: level = Level.toLevel(levelName, null); Line 54: if (level == null) { Line 59: FileAppender fa = null; Line 60: try { Line 61: fa = new FileAppender(new PatternLayout("%d %-5p [%c] %m%n"), fileName, true); Line 62: fa.setThreshold(level); Line 63: } catch (SecurityException | IOException ex) { > To be able to report user friendly error instead of stacktrace and this cal once again, the difference between: public static void addFileAppender(String fileName, String levelName) { try { logic } catch (SecurityException | IOException ex) { throw new RuntimException("Cannot add appender X at level Y"); } } how come you get IOException if it is not of file, how come you get SecurityException if the problem is not the file? why do you care if it is the file or anything else, bottom line you fail to add the appender. I guess I will never understand. Line 64: throw new IllegalArgumentException( Line 65: String.format("Error accessing log file '%s': '%s'", fileName, ex.getMessage()), Line 66: ex); Line 67: } -- 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: 2 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
