Ali Alsuliman has posted comments on this change. Change subject: [NO ISSUE] Tracer improvements ......................................................................
Patch Set 3: (18 comments) https://asterix-gerrit.ics.uci.edu/#/c/3064/3/asterixdb/asterix-app/src/main/java/org/apache/asterix/hyracks/bootstrap/NCApplication.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/hyracks/bootstrap/NCApplication.java: PS3, Line 186: } Could we remove this file from the commit now that there are no changes to it? https://asterix-gerrit.ics.uci.edu/#/c/3064/3/asterixdb/asterix-app/src/main/resources/log4j2.xml File asterixdb/asterix-app/src/main/resources/log4j2.xml: PS3, Line 22: "600" Well, if you're changing it to 600, then that's basically log4j's TRACE which has the same level. Either remove this custom level and use log4j's TRACE, or really give it a different custom level than provided by log4j. PS3, Line 32: "%m%n" I'm assuming you don't want the comma in the console output. PS3, Line 35: "%m%n" Shouldn't this have a comma in between? we should try to keep this .xml config file and the code configuring the logging in sync as much as possible, at least to avoid confusion. PS3, Line 46: name="org.apache.hyracks.util.trace" Change this to match the one in the code (i.e. org.apache.hyracks.util.trace.Tracer.Traces). We only want the "traceLog" logger embedded inside Tracer to use this setup (Also Tracer.java has its own LOGGER which should log normally like any other loggers). https://asterix-gerrit.ics.uci.edu/#/c/3064/3/asterixdb/asterix-app/src/test/resources/log4j2-asterixdb-test.xml File asterixdb/asterix-app/src/test/resources/log4j2-asterixdb-test.xml: PS3, Line 31: "%m%n" I'm assuming you don't want the comma in the console output. PS3, Line 41: org.apache.hyracks.util.trace.Tracer Change this to "org.apache.hyracks.util.trace.Tracer.Traces". You don't want the Tracer.java LOGGER to use this setup. You want the embedded traceLog logger to use this setup. PS3, Line 41: level="TRACER" I don't think this will work. How is this file referencing a custom level defined in another file? https://asterix-gerrit.ics.uci.edu/#/c/3064/3/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-nc/src/main/java/org/apache/hyracks/control/nc/NCLogConfigurationFactory.java File hyracks-fullstack/hyracks/hyracks-control/hyracks-control-nc/src/main/java/org/apache/hyracks/control/nc/NCLogConfigurationFactory.java: PS3, Line 82: 500 500 here but 600 in the .xml file? Also, 500 is already log4j's DEBUG. We should either use DEBUG or give this custom level TRACER a different level. https://asterix-gerrit.ics.uci.edu/#/c/3064/3/hyracks-fullstack/hyracks/hyracks-util/src/main/java/org/apache/hyracks/util/trace/ITracer.java File hyracks-fullstack/hyracks/hyracks-util/src/main/java/org/apache/hyracks/util/trace/ITracer.java: PS3, Line 85: @Override Can we remove this file from the commit? https://asterix-gerrit.ics.uci.edu/#/c/3064/3/hyracks-fullstack/hyracks/hyracks-util/src/main/java/org/apache/hyracks/util/trace/Tracer.java File hyracks-fullstack/hyracks/hyracks-util/src/main/java/org/apache/hyracks/util/trace/Tracer.java: PS3, Line 32: import static org.apache.logging.log4j.Level.INFO; Can we also remove this import after reverting the below changes? PS3, Line 33: import static org.apache.logging.log4j.Level.TRACE; Can we remove this unused import? PS3, Line 38: Tracer Could you make all of the below variables private? PS3, Line 42: 500 500 here but 600 in the .xml file? Also 500 is log4j's DEBUG PS3, Line 48: TraceCategoryRegistry Could you make this final? PS3, Line 54: log(INFO, This is effectively the same as the original one, LOGGER.info(). Can we revert this? PS3, Line 59: traceLog.log(TRACE_LOG_LEVEL, "["); I think this should be part of the layout definition (whether you're doing it in the code programmatically or in the .xml file). Have you tried "header" and it didn't work? PS3, Line 70: log(INFO, Same here -- To view, visit https://asterix-gerrit.ics.uci.edu/3064 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I81c158fcd17927d65e7b501345fdbc98001ba86a Gerrit-PatchSet: 3 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Ian Maxon <ima...@apache.org> Gerrit-Reviewer: Ali Alsuliman <ali.al.solai...@gmail.com> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Ian Maxon <ima...@apache.org> Gerrit-Reviewer: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Gerrit-HasComments: Yes
