[GitHub] [hadoop] virajjasani commented on pull request #5315: HADOOP-18206 Cleanup the commons-logging references and restrict its usage in future
virajjasani commented on PR #5315: URL: https://github.com/apache/hadoop/pull/5315#issuecomment-1423060494 @steveloughran, please let me know if the above explanation looks good to you. If you have more comments, I would be happy to address them. Thank you -- 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: common-issues-unsubscr...@hadoop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] virajjasani commented on pull request #5315: HADOOP-18206 Cleanup the commons-logging references and restrict its usage in future
virajjasani commented on PR #5315: URL: https://github.com/apache/hadoop/pull/5315#issuecomment-1421255334 If we try to explicitly manage external/transitive dependency for commons-logging, a large chunk of our own tests would fail, like this: https://user-images.githubusercontent.com/34790606/217333150-09c7e8aa-44c3-49e0-8e28-b1324799ae63.png;> -- 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: common-issues-unsubscr...@hadoop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] virajjasani commented on pull request #5315: HADOOP-18206 Cleanup the commons-logging references and restrict its usage in future
virajjasani commented on PR #5315: URL: https://github.com/apache/hadoop/pull/5315#issuecomment-1419383687 > just noticed that http-components pulls in commons logging so things which depend on that still need to export it. question then becomes: do we want to explicitly manage the dependency there? > > ``` > [info] | | +-org.apache.hadoop:hadoop-azure:3.3.5 > [info] | | | +-com.microsoft.azure:azure-storage:7.0.1 > [info] | | | | +-com.microsoft.azure:azure-keyvault-core:1.0.0 > [info] | | | | +-org.slf4j:slf4j-api:1.7.12 (evicted by: 2.0.6) > [info] | | | | +-org.slf4j:slf4j-api:2.0.6 > [info] | | | | > [info] | | | +-org.apache.hadoop.thirdparty:hadoop-shaded-guava:1.1.1 > [info] | | | +-org.apache.httpcomponents:httpclient:4.5.13 (evicted by: 4.5.14) > [info] | | | +-org.apache.httpcomponents:httpclient:4.5.14 > [info] | | | | +-commons-codec:commons-codec:1.11 (evicted by: 1.15) > [info] | | | | +-commons-codec:commons-codec:1.15 > [info] | | | | +-commons-logging:commons-logging:1.2 > [info] | | | | +-org.apache.httpcomponents:httpcore:4.4.16 > ``` Good question, I actually removed them in the initial iteration, however while running some testing, it failed. Because many dependencies still need commons-logging and hence we see bunch of CNFE for few dependencies. I had to keep the changes and let dependencies pull in commons-logging as they would like. The only thing we can change is, remove commons-logging references from our codebase. Which we are doing with this PR. -- 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: common-issues-unsubscr...@hadoop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] virajjasani commented on pull request #5315: HADOOP-18206 Cleanup the commons-logging references and restrict its usage in future
virajjasani commented on PR #5315: URL: https://github.com/apache/hadoop/pull/5315#issuecomment-1418522125 This PR could be ported to branch-3.3 as well. I can create follow up PR for the same. Hence, for that reason, I have kept a couple of IA.Public/IS.Evolving and marked them `@Deprecated`. I will create separate sub-task to remove them (only from trunk, not from branch-3.3). -- 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: common-issues-unsubscr...@hadoop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] virajjasani commented on pull request #5315: HADOOP-18206 Cleanup the commons-logging references and restrict its usage in future
virajjasani commented on PR #5315: URL: https://github.com/apache/hadoop/pull/5315#issuecomment-1414240581 @steveloughran @jojochuang @Apache9 I have addressed all review comments so far. For the couple of IA.Public/IS.Evolving methods, instead of removing, I kept them with `@Deprecated`, will create another sub-task to remove those methods. Can you please take another look? -- 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: common-issues-unsubscr...@hadoop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] virajjasani commented on pull request #5315: HADOOP-18206 Cleanup the commons-logging references and restrict its usage in future
virajjasani commented on PR #5315: URL: https://github.com/apache/hadoop/pull/5315#issuecomment-1409146994 > is there a way the log4j refs can be kept inside MetricsLoggerTask Done -- 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: common-issues-unsubscr...@hadoop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] virajjasani commented on pull request #5315: HADOOP-18206 Cleanup the commons-logging references and restrict its usage in future
virajjasani commented on PR #5315: URL: https://github.com/apache/hadoop/pull/5315#issuecomment-1409097037 FWIW, the parent Jira HADOOP-16206 is also marked as "incompatible change" hence I believe we would not be able to backport this whole work to branch-3.x anyways. -- 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: common-issues-unsubscr...@hadoop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] virajjasani commented on pull request #5315: HADOOP-18206 Cleanup the commons-logging references and restrict its usage in future
virajjasani commented on PR #5315: URL: https://github.com/apache/hadoop/pull/5315#issuecomment-1409002069 Moreover, the main goal of this sub-task is to remove only commons-logging references hence I think it would be better to replace Datanode/Namenode async logger with SLF4J only (with log4j.properties including async logger). > If those log4j refs can be hidden entirely within MetricsLoggerTask then some reflection games could be done there perhaps. We would have to pass in the name of the logger and let that class do any reflection-based look up of the actual log. We have sub-task to introduce hadoop-logging module altogether. The plan is to move all log4j appender and other logic to that module and have reflection do the work. All hadoop modules to only extend hadoop-logging module. But this sub-task is only related to removal of commons-logging. And places in hdfs would not be functional even as of today (without this patch) if client doesn't include Log4J in the classpath (because dynamically adding appender is part of hdfs own logic). -- 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: common-issues-unsubscr...@hadoop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] virajjasani commented on pull request #5315: HADOOP-18206 Cleanup the commons-logging references and restrict its usage in future
virajjasani commented on PR #5315: URL: https://github.com/apache/hadoop/pull/5315#issuecomment-1408995424 > I've reviewed where log4j gets used in the production code. I do worry that today there's enough reflection that if deployed with logback all is good, but now log4j is required. Even without this patch, if we don't have log4j in classpath, hdfs cannot come up right? For instance, `MetricsLoggerTask` is adding appenders dynamically and for that, we do need appender classes to be present in the classpath, meaning log4j is mandatory in the classpath. However, I am going to cover all dynamic async appenders and replace with log4j properties, then we can directly use SLF4J and remove all Log4J references in the codebase. Does this sound good? -- 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: common-issues-unsubscr...@hadoop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] virajjasani commented on pull request #5315: HADOOP-18206 Cleanup the commons-logging references and restrict its usage in future
virajjasani commented on PR #5315: URL: https://github.com/apache/hadoop/pull/5315#issuecomment-1406741496 > we are changing the public API by changing the signature of cleanup(Log log, java.io.Closeable... closeables) to cleanup(Logger log, java.io.Closeable... closeables). This is why I don't see that removing the method is any worse. The salt justification for retention would be "it is slightly easier to migrate if you already have an slf4j instance" Valid point. I am fine either ways. > we MUST NOT have hard coded log4j references in production code, especially not anything which may be used client-side. Server side we may have a bit more flexibility but I think it would be bad too put those log for Joe references in as (a) that code may be copied into client side classes and (b) I feel making changes now we may as well be more independent here. I wish we didn't have log4j hardcoded but we have it at several places already. But no worries, replacing them with slf4j before we finally migrate to lo4j2 would be my next set of sub-tasks. I need to tackle them carefully because we have too much dynamically updated appenders, which is not good, but I would move forward with the consensus we have on parent Jira HADOOP-16206 -- 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: common-issues-unsubscr...@hadoop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] virajjasani commented on pull request #5315: HADOOP-18206 Cleanup the commons-logging references and restrict its usage in future
virajjasani commented on PR #5315: URL: https://github.com/apache/hadoop/pull/5315#issuecomment-1405594849 @jojochuang merge conflicts resolved with latest sync from trunk -- 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: common-issues-unsubscr...@hadoop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] virajjasani commented on pull request #5315: HADOOP-18206 Cleanup the commons-logging references and restrict its usage in future
virajjasani commented on PR #5315: URL: https://github.com/apache/hadoop/pull/5315#issuecomment-1397489869 Javadoc warnings for common and spotbug warnings for mapreduce modules are not relevant to this change. -- 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: common-issues-unsubscr...@hadoop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] virajjasani commented on pull request #5315: HADOOP-18206 Cleanup the commons-logging references and restrict its usage in future
virajjasani commented on PR #5315: URL: https://github.com/apache/hadoop/pull/5315#issuecomment-1397385744 bummer, we can't completely get rid of it as commons-configuration needs it, so we will have to keep the version in our classpath but we won't use it in the codebase. -- 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: common-issues-unsubscr...@hadoop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
