----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4732/#review6937 -----------------------------------------------------------
/trunk/src/java/org/apache/hcatalog/common/HCatLogUtil.java <https://reviews.apache.org/r/4732/#comment15482> With this everything will be logged as HCatLogUtil.class which beats the point of logging messages for debugging purpose. /trunk/src/java/org/apache/hcatalog/common/HCatLogUtil.java <https://reviews.apache.org/r/4732/#comment15480> It would be cleaner and configurable if formatting is part of log4j.xml of log4j.properties and not part of code. /trunk/src/java/org/apache/hcatalog/common/HCatLogUtil.java <https://reviews.apache.org/r/4732/#comment15481> It would be better to do String concatenation than string format. String.format is very costly as it does pattern matching. https://issues.apache.org/jira/browse/MAPREDUCE-1533 /trunk/src/java/org/apache/hcatalog/data/HCatRecordObjectInspectorFactory.java <https://reviews.apache.org/r/4732/#comment15478> You will have to do a isDebugEnabled() before the debug call. The main intention of it is to avoid the cost of string concatenation/parameter construction. http://logging.apache.org/log4j/1.2/manual.html#performance /trunk/src/java/org/apache/hcatalog/mapreduce/HCatRecordReader.java <https://reviews.apache.org/r/4732/#comment15484> Log.warn("some msg", e) would log both e.getMessage and e's strack trace. Logging them separately is redundant. There are many instances of this in the code which can be cleaned up. - Rohini On 2012-04-16 06:31:43, Vandana Ayyalasomayajula wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4732/ > ----------------------------------------------------------- > > (Updated 2012-04-16 06:31:43) > > > Review request for hcatalog, Sushanth Sowmyan, Francis Liu, and Thomas. > > > Summary > ------- > > We need to do the following things: > > + Use log4j across the board > + Introduce a class with methods to help logging, refactoring away the bunch > of these functions in HCatUtil right now > + Instrument debug logging with isLogEnabled checks on the log level > + remove all instances of commented code whose purpose was for debug logs > > > This addresses bug hcatalog-68. > https://issues.apache.org/jira/browse/hcatalog-68 > > > Diffs > ----- > > /trunk/ivy.xml 1325838 > > /trunk/src/java/org/apache/hcatalog/cli/SemanticAnalysis/HCatSemanticAnalyzer.java > 1325838 > /trunk/src/java/org/apache/hcatalog/common/HCatLogUtil.java PRE-CREATION > /trunk/src/java/org/apache/hcatalog/common/HCatUtil.java 1325838 > > /trunk/src/java/org/apache/hcatalog/data/HCatRecordObjectInspectorFactory.java > 1325838 > /trunk/src/java/org/apache/hcatalog/data/HCatRecordSerDe.java 1325838 > /trunk/src/java/org/apache/hcatalog/data/JsonSerDe.java 1325838 > > /trunk/src/java/org/apache/hcatalog/mapreduce/DefaultOutputCommitterContainer.java > 1325838 > /trunk/src/java/org/apache/hcatalog/mapreduce/HCatOutputFormat.java 1325838 > /trunk/src/java/org/apache/hcatalog/mapreduce/HCatRecordReader.java 1325838 > /trunk/src/java/org/apache/hcatalog/mapreduce/HCatSplit.java 1325838 > /trunk/src/java/org/apache/hcatalog/mapreduce/InitializeInput.java 1325838 > /trunk/src/test/org/apache/hcatalog/data/HCatDataCheckUtil.java 1325838 > /trunk/src/test/org/apache/hcatalog/data/TestJsonSerDe.java 1325838 > > Diff: https://reviews.apache.org/r/4732/diff > > > Testing > ------- > > Unit tests pass. Log messages verified from test logs. > > > Thanks, > > Vandana > >
