You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@gobblin.apache.org by GitBox <gi...@apache.org> on 2020/08/14 23:14:12 UTC

[GitHub] [incubator-gobblin] ZihanLi58 opened a new pull request #3081: [GOBBLIN-1235]Migrate Log4J to SLF4J to provide a clean log environment for downstream users

ZihanLi58 opened a new pull request #3081:
URL: https://github.com/apache/incubator-gobblin/pull/3081


   … still use log4j as implementation
   
   Dear Gobblin maintainers,
   
   Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!
   
   
   ### JIRA
   - [ ] My PR addresses the following [Gobblin JIRA](https://issues.apache.org/jira/browse/GOBBLIN/) issues and references them in the PR title. For example, "[GOBBLIN-XXX] My Gobblin PR"
       - https://issues.apache.org/jira/browse/GOBBLIN-1235
   
   
   ### Description
   - [ ] Here are some details about my PR, including screenshots (if applicable):
   change gradle dependencies to migrate log4j to slf4j, and for unit test we still use log4j as log implementation
   By applying this change, to make job can still running on hadoop or still use log4j as log implementation, user will need to exclude 'org.slf4j', module: 'log4j-over-slf4j' from their job to avoid endless loop between 'log4j-over-slf4j' and 'slf4j-log4j12'
   
   ### Tests
   - [ ] My PR adds the following unit tests __OR__ does not need testing for this extremely good reason:
   Test on hadoop that this can work, local deploy service and make sure they can use log4j2 as log implementation.
   
   ### Commits
   - [ ] My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
       1. Subject is separated from body by a blank line
       2. Subject is limited to 50 characters
       3. Subject does not end with a period
       4. Subject uses the imperative mood ("add", not "adding")
       5. Body wraps at 72 characters
       6. Body explains "what" and "why", not "how"
   
   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-gobblin] autumnust commented on a change in pull request #3081: [GOBBLIN-1235]Migrate Log4J to SLF4J to provide a clean log environment for downstream users

Posted by GitBox <gi...@apache.org>.
autumnust commented on a change in pull request #3081:
URL: https://github.com/apache/incubator-gobblin/pull/3081#discussion_r477603810



##########
File path: gradle/scripts/dependencyDefinitions.gradle
##########
@@ -180,13 +180,16 @@ ext.externalDependency = [
     'reactivex': 'io.reactivex.rxjava2:rxjava:2.1.0',
     "slf4j": [
         "org.slf4j:slf4j-api:" + slf4jVersion,
-        "org.slf4j:slf4j-log4j12:" + slf4jVersion,
+        "org.slf4j:log4j-over-slf4j:" + slf4jVersion,
         "org.slf4j:jcl-over-slf4j:" + slf4jVersion
     ],
     "log4j": [

Review comment:
       Gotcha. You may also want to check the conflicts, after which I can merge this so it could be tested in larger scope. 




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-gobblin] autumnust commented on a change in pull request #3081: [GOBBLIN-1235]Migrate Log4J to SLF4J to provide a clean log environment for downstream users

Posted by GitBox <gi...@apache.org>.
autumnust commented on a change in pull request #3081:
URL: https://github.com/apache/incubator-gobblin/pull/3081#discussion_r477541636



##########
File path: gradle/scripts/dependencyDefinitions.gradle
##########
@@ -180,13 +180,16 @@ ext.externalDependency = [
     'reactivex': 'io.reactivex.rxjava2:rxjava:2.1.0',
     "slf4j": [
         "org.slf4j:slf4j-api:" + slf4jVersion,
-        "org.slf4j:slf4j-log4j12:" + slf4jVersion,
+        "org.slf4j:log4j-over-slf4j:" + slf4jVersion,
         "org.slf4j:jcl-over-slf4j:" + slf4jVersion
     ],
     "log4j": [

Review comment:
       is this block still needed if you removed references ? 




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-gobblin] autumnust commented on a change in pull request #3081: [GOBBLIN-1235]Migrate Log4J to SLF4J to provide a clean log environment for downstream users

Posted by GitBox <gi...@apache.org>.
autumnust commented on a change in pull request #3081:
URL: https://github.com/apache/incubator-gobblin/pull/3081#discussion_r470928416



##########
File path: gobblin-admin/build.gradle
##########
@@ -34,6 +34,7 @@ dependencies {
     compile externalDependency.jodaTime
 
     testCompile externalDependency.testng
+    testCompile externalDependency.slf4jToLog4j

Review comment:
       why this is only added for test-compile (isn't that the case we use slf4j all over the places? 




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-gobblin] asfgit closed pull request #3081: [GOBBLIN-1235]Migrate Log4J to SLF4J to provide a clean log environment for downstream users

Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #3081:
URL: https://github.com/apache/incubator-gobblin/pull/3081


   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-gobblin] autumnust commented on a change in pull request #3081: [GOBBLIN-1235]Migrate Log4J to SLF4J to provide a clean log environment for downstream users

Posted by GitBox <gi...@apache.org>.
autumnust commented on a change in pull request #3081:
URL: https://github.com/apache/incubator-gobblin/pull/3081#discussion_r470936412



##########
File path: gradle/scripts/dependencyDefinitions.gradle
##########
@@ -180,13 +180,16 @@ ext.externalDependency = [
     'reactivex': 'io.reactivex.rxjava2:rxjava:2.1.0',
     "slf4j": [
         "org.slf4j:slf4j-api:" + slf4jVersion,
-        "org.slf4j:slf4j-log4j12:" + slf4jVersion,
+        "org.slf4j:log4j-over-slf4j:" + slf4jVersion,

Review comment:
       OK now I understand it through reading this [link](http://www.slf4j.org/legacy.html#:~:text=SLF4J%20ship%20with%20a%20module,simply%20by%20replacing%20the%20log4j.) ! 
   

##########
File path: gobblin-admin/build.gradle
##########
@@ -34,6 +34,7 @@ dependencies {
     compile externalDependency.jodaTime
 
     testCompile externalDependency.testng
+    testCompile externalDependency.slf4jToLog4j

Review comment:
       OK I need a bit more clarification here: 
   I think the purpose of adding this line is to add the binding in these test classes to make the log4j being picked as the implementation of slf4j ? 




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-gobblin] ZihanLi58 commented on a change in pull request #3081: [GOBBLIN-1235]Migrate Log4J to SLF4J to provide a clean log environment for downstream users

Posted by GitBox <gi...@apache.org>.
ZihanLi58 commented on a change in pull request #3081:
URL: https://github.com/apache/incubator-gobblin/pull/3081#discussion_r477542568



##########
File path: gradle/scripts/dependencyDefinitions.gradle
##########
@@ -180,13 +180,16 @@ ext.externalDependency = [
     'reactivex': 'io.reactivex.rxjava2:rxjava:2.1.0',
     "slf4j": [
         "org.slf4j:slf4j-api:" + slf4jVersion,
-        "org.slf4j:slf4j-log4j12:" + slf4jVersion,
+        "org.slf4j:log4j-over-slf4j:" + slf4jVersion,
         "org.slf4j:jcl-over-slf4j:" + slf4jVersion
     ],
     "log4j": [

Review comment:
       We still use log4j in unit test, and I keep the dependency for log4j in gobblin-aws since we specifically use appender which slf4j does not support.




----------------------------------------------------------------
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:
users@infra.apache.org