zabetak commented on code in PR #4692:
URL: https://github.com/apache/hive/pull/4692#discussion_r1325531413
##########
accumulo-handler/src/java/org/apache/hadoop/hive/accumulo/serde/AccumuloSerDe.java:
##########
@@ -54,6 +54,11 @@ public class AccumuloSerDe extends AbstractSerDe {
private static final Logger log =
LoggerFactory.getLogger(AccumuloSerDe.class);
+ @Override
+ public Logger getClassLogger() {
+ return log;
+ }
+
Review Comment:
Having overridable instance methods return static fields is uncommon and
complicates extensibility. Let's avoid this pattern.
##########
druid-handler/src/test/org/apache/hadoop/hive/druid/QTestDruidSerDe.java:
##########
@@ -27,12 +27,16 @@
import org.apache.druid.query.metadata.metadata.SegmentAnalysis;
import org.apache.druid.query.metadata.metadata.SegmentMetadataQuery;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/**
* Druid SerDe to be used in tests.
*/
public class QTestDruidSerDe extends DruidSerDe {
+ public static final Logger log =
LoggerFactory.getLogger(QTestDruidSerDe.class.getName());
Review Comment:
There are no log messages in this class (and in some other SerDe classes) so
there is no reason to introduce unused logger variables in those.
##########
contrib/src/java/org/apache/hadoop/hive/contrib/serde2/RegexSerDe.java:
##########
@@ -76,6 +76,8 @@
RegexSerDe.INPUT_REGEX_CASE_SENSITIVE })
public class RegexSerDe extends AbstractEncodingAwareSerDe {
+ public static final Logger log =
LoggerFactory.getLogger(RegexSerDe.class.getName());
Review Comment:
Since we are introducing a `static final` logger in every class there is no
reason to make it public. The intention is to use them inside each class so
every logger should be private.
Also according to the naming conventions in the Java specification
(https://docs.oracle.com/javase/specs/jls/se20/html/jls-6.html#jls-6.1)
constants should be all uppercase.
##########
serde/src/java/org/apache/hadoop/hive/serde2/AbstractSerDe.java:
##########
@@ -189,7 +192,7 @@ public Optional<Configuration> getConfiguration() {
@Override
public String toString() {
- return "AbstractSerDe [log=" + log + ", configuration=" + configuration +
", properties=" + properties
+ return "AbstractSerDe [log=" + getClassLogger() + ", configuration=" +
configuration + ", properties=" + properties
Review Comment:
Let's drop `log` completely from here. This is purely for debugging purposes
and I don't think it helps much to see the logger especially now that we are
turning everything to static fields.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscr...@hive.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscr...@hive.apache.org
For additional commands, e-mail: gitbox-h...@hive.apache.org
