saintstack commented on a change in pull request #1640:
URL: https://github.com/apache/hbase/pull/1640#discussion_r421937064
##########
File path: hbase-assembly/pom.xml
##########
@@ -318,5 +318,18 @@
<artifactId>jaxws-ri</artifactId>
<type>pom</type>
</dependency>
+ <!--
+ Include the log framework here.
+ For other sub modules, we only declare slf4j-api as a compile dependency,
+ so here we must pull in the real logging framework to actually log to
log4j.
+ -->
Review comment:
What happens when we run in-situ... i.e. build and then do
./bin/start-hbase.sh? Does it produce logs?
##########
File path: hbase-assembly/pom.xml
##########
@@ -318,5 +318,18 @@
<artifactId>jaxws-ri</artifactId>
<type>pom</type>
</dependency>
+ <!--
+ Include the log framework here.
+ For other sub modules, we only declare slf4j-api as a compile dependency,
+ so here we must pull in the real logging framework to actually log to
log4j.
+ -->
Review comment:
This is a good note. Should it go elsewhere, in the top-level pom for
instance? (Maybe it is there already... Let me keep going)
##########
File path: hbase-archetypes/hbase-client-project/pom.xml
##########
@@ -43,31 +43,35 @@
<dependency>
<groupId>org.apache.hbase</groupId>
<artifactId>hbase-testing-util</artifactId>
- <version>${project.version}</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.apache.hbase</groupId>
<artifactId>hbase-common</artifactId>
- <version>${project.version}</version>
Review comment:
Good
##########
File path: hbase-shaded/pom.xml
##########
@@ -51,15 +51,14 @@
<dependency>
<groupId>org.apache.hbase</groupId>
<artifactId>hbase-resource-bundle</artifactId>
- <version>${project.version}</version>
<optional>true</optional>
</dependency>
- <!-- log4j has to be non-optional for Hadoop 2 atleast -->
+ <!-- put the log implementations to optional -->
Review comment:
This is targeted at hbase2 which is to run against hadoop2? This comment
that it can't be non-optional is fixed now? Not needed?
##########
File path: pom.xml
##########
@@ -2861,6 +2917,14 @@
<groupId>org.codehause.jackson</groupId>
<artifactId>jackson-mapper-asl</artifactId>
</exclusion>
+ <exclusion>
+ <groupId>org.slf4j</groupId>
+ <artifactId>slf4j-log4j12</artifactId>
+ </exclusion>
+ <exclusion>
+ <groupId>log4j</groupId>
+ <artifactId>log4j</artifactId>
+ </exclusion>
Review comment:
Painful.
##########
File path: hbase-server/pom.xml
##########
@@ -481,15 +473,6 @@
<artifactId>httpcore</artifactId>
<scope>test</scope>
</dependency>
- <!-- commons-logging is used by HBTU to monkey with log levels
- have to put it at compile scope because Hadoop's IOUtils uses it
- both for hadoop 2.7 and 3.0, so we'll fail at compile if it's at test
scope.
- -->
- <dependency>
- <groupId>commons-logging</groupId>
- <artifactId>commons-logging</artifactId>
- <scope>compile</scope>
- </dependency>
Review comment:
Fixed?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
