> On March 3, 2018, 4:41 a.m., Tomás Fernández Löbbe wrote:
> > LGTM. I think you need to add org.apache.logging.log4j.** to forbidden APIs
> 
> Erick Erickson wrote:
>     It turns out not. Thanks to Shawn I now know the shim is used, which 
> means we can (and do) still use log4j calls in the code.
> 
> Tomás Fernández Löbbe wrote:
>     Not entierly sure. We **don't** do log4j calls in the code in general, we 
> have that explicitly forbidden in forbidden APIS today, and code that does 
> something with log4j has to supress that. Developers must instead use slf4j 
> APIs. I don't believe that's changing now with log4j2, or does it?
> 
> Shawn Heisey wrote:
>     ```
>     Not entierly sure. We don't do log4j calls in the code in general, we 
> have that explicitly forbidden in forbidden APIS today, and code that does 
> something with log4j has to supress that. Developers must instead use slf4j 
> APIs. I don't believe that's changing now with log4j2, or does it?
>     ```
>     
>     Solr only uses log4j code in one place -- our log watcher for the admin 
> UI.  This probably has a notation to disable the forbidden API check.  Lucene 
> doesn't use any logging at all as far as I know.
>     
>     The reason that we need the 1.2 compatibility shim is not for Solr or 
> Lucene.  It's for dependencies.  One dependency that I know for sure uses it 
> is zookeeper.  There might be others, but I haven't done an exhaustive check 
> to find out.
> 
> Tomás Fernández Löbbe wrote:
>     That is exactly my point. Solr doesn't use log4j in the code and it's 
> forbidden. We need to include the log4j2 packages in the list of forbidden so 
> that developers don't try to use it (unless they know what they are doing and 
> explicitly remove the forbidden check)
> 
> Shawn Heisey wrote:
>     Sounds like we are in violent agreement, then. :)  Yes, we should make a 
> general rule of forbidding log4j usage, both 1.2 and 2.x, and provide an 
> exception for the one place where we use it intentionally.  Am I correct in 
> thinking that forbidden APIs won't complain about usage in dependencies, just 
> the class files that our code builds?
> 
> Tomás Fernández Löbbe wrote:
>     Right. And we already have lines for forbidding 1.2 jog4j and JUL. My 
> point is that we need to add the new packages there. See file "solr.txt" 
> under lucene/tools/forbiddenApis
>     ```
>     @defaultMessage Use slf4j classes instead
>     org.apache.log4j.**
>     java.util.logging.**
>     ```

Maybe raise a separate JIRA? IIRC I tried this and it failed precommit 
miserably, didn't pursue as this JIRA is big enough as it is......


- Erick


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65888/#review198580
-----------------------------------------------------------


On March 3, 2018, 2:51 a.m., Varun Thacker wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65888/
> -----------------------------------------------------------
> 
> (Updated March 3, 2018, 2:51 a.m.)
> 
> 
> Review request for lucene.
> 
> 
> Repository: lucene-solr
> 
> 
> Description
> -------
> 
> Upgrade Solr to use Log4J2
> 
> 
> Diffs
> -----
> 
>   lucene/CHANGES.txt e3799bc9d5 
>   lucene/common-build.xml 4fa59ac936 
>   lucene/ivy-versions.properties 5ab36ddfa2 
>   solr/CHANGES.txt 05f4f560bd 
>   solr/bin/install_solr_service.sh b82957144d 
>   solr/bin/solr 4e178de945 
>   solr/bin/solr.cmd dcff0c6af7 
>   solr/bin/solr.in.cmd bfb33e0e9d 
>   solr/bin/solr.in.sh e7478cdf5c 
>   solr/contrib/clustering/src/test-files/log4j.properties b5216db8b2 
>   solr/contrib/clustering/src/test-files/log4j2.xml PRE-CREATION 
>   solr/contrib/dataimporthandler/src/test-files/log4j.properties d3ea4deafc 
>   solr/contrib/dataimporthandler/src/test-files/log4j2.xml PRE-CREATION 
>   solr/contrib/ltr/src/test-files/log4j.properties d86c6988d5 
>   solr/contrib/ltr/src/test-files/log4j2.xml PRE-CREATION 
>   solr/core/ivy.xml ff4fa48679 
>   
> solr/core/src/java/org/apache/solr/client/solrj/embedded/JettySolrRunner.java 
> 23a8dc1eb3 
>   solr/core/src/java/org/apache/solr/handler/admin/LoggingHandler.java 
> 122d2cbf8b 
>   solr/core/src/java/org/apache/solr/logging/LogWatcher.java c510590282 
>   solr/core/src/java/org/apache/solr/logging/log4j/EventAppender.java 
> ff2876fb2f 
>   solr/core/src/java/org/apache/solr/logging/log4j/Log4jInfo.java dfd3dde74a 
>   solr/core/src/java/org/apache/solr/logging/log4j/Log4jWatcher.java 
> 04fa5fb1d8 
>   solr/core/src/java/org/apache/solr/logging/log4j/package-info.java 
> f78953385c 
>   solr/core/src/java/org/apache/solr/logging/log4j2/Log4j2Watcher.java 
> PRE-CREATION 
>   solr/core/src/java/org/apache/solr/servlet/SolrDispatchFilter.java 
> 4d944d239b 
>   solr/core/src/java/org/apache/solr/util/SolrCLI.java 266ef3a28a 
>   solr/core/src/java/org/apache/solr/util/SolrLogLayout.java a60ada828b 
>   solr/core/src/java/org/apache/solr/util/StartupLoggingUtils.java c582eff4c0 
>   solr/core/src/test-files/log4j.properties 969439a228 
>   solr/core/src/test-files/log4j2.xml PRE-CREATION 
>   solr/core/src/test/org/apache/solr/handler/RequestLoggingTest.java 
> 4c780ccda4 
>   solr/core/src/test/org/apache/solr/handler/admin/LoggingHandlerTest.java 
> 555c1376a5 
>   solr/core/src/test/org/apache/solr/logging/log4j2/Log4j2WatcherTest.java 
> PRE-CREATION 
>   solr/core/src/test/org/apache/solr/util/TestSolrCLIRunExample.java 
> 89008517f8 
>   solr/example/README.txt 562c256377 
>   solr/example/example-DIH/solr/db/conf/solrconfig.xml 1ffbbe817f 
>   solr/example/example-DIH/solr/mail/conf/solrconfig.xml 770b0fd870 
>   solr/example/example-DIH/solr/solr/conf/solrconfig.xml 3f00141340 
>   solr/example/resources/log4j.properties 02f91c5dae 
>   solr/example/resources/log4j2.xml PRE-CREATION 
>   solr/licenses/audience-annotations-0.7.0.jar.sha1 PRE-CREATION 
>   solr/licenses/audience-annotations-LICENSE-ASL.txt PRE-CREATION 
>   solr/licenses/audience-annotations-NOTICE.txt PRE-CREATION 
>   solr/licenses/disruptor-3.4.0.jar.sha1 PRE-CREATION 
>   solr/licenses/disruptor-LICENSE-ASL.txt PRE-CREATION 
>   solr/licenses/disruptor-NOTICE.txt PRE-CREATION 
>   solr/licenses/log4j-1.2-api-2.10.0.jar.sha1 PRE-CREATION 
>   solr/licenses/log4j-1.2.17.jar.sha1 383110e29f 
>   solr/licenses/log4j-api-2.10.0.jar.sha1 PRE-CREATION 
>   solr/licenses/log4j-api-LICENSE-ASL.txt PRE-CREATION 
>   solr/licenses/log4j-api-NOTICE.txt PRE-CREATION 
>   solr/licenses/log4j-core-2.10.0.jar.sha1 PRE-CREATION 
>   solr/licenses/log4j-core-LICENSE-ASL.txt PRE-CREATION 
>   solr/licenses/log4j-core-NOTICE.txt PRE-CREATION 
>   solr/licenses/log4j-slf4j-LICENSE-ASL.txt PRE-CREATION 
>   solr/licenses/log4j-slf4j-NOTICE.txt PRE-CREATION 
>   solr/licenses/log4j-slf4j-impl-2.10.0.jar.sha1 PRE-CREATION 
>   solr/licenses/slf4j-log4j12-1.7.24.jar.sha1 b8ec050172 
>   solr/server/README.txt 228f4d467b 
>   solr/server/ivy.xml c9b3a73014 
>   solr/server/resources/log4j.properties 9f9c4a0f74 
>   solr/server/resources/log4j2.xml PRE-CREATION 
>   solr/server/scripts/cloud-scripts/log4j.properties 5f2ae18574 
>   solr/server/scripts/cloud-scripts/log4j2.xml PRE-CREATION 
>   solr/server/scripts/cloud-scripts/snapshotscli.sh f885721f66 
>   solr/server/scripts/cloud-scripts/zkcli.bat c5d7b72948 
>   solr/server/scripts/cloud-scripts/zkcli.sh bd971e9ee4 
>   solr/server/solr/configsets/sample_techproducts_configs/conf/solrconfig.xml 
> 1b2563662e 
>   solr/solr-ref-guide/ivy.xml adefe2ced5 
>   solr/solr-ref-guide/src/configuring-logging.adoc 8984744e55 
>   solr/solr-ref-guide/src/solr-control-script-reference.adoc 8ca14ea476 
>   solr/solr-ref-guide/src/taking-solr-to-production.adoc ca0f4ebe76 
>   solr/solrj/ivy.xml 3637bc34bd 
>   solr/solrj/src/test-files/log4j.properties dae4f6f418 
>   solr/solrj/src/test-files/log4j2.xml PRE-CREATION 
>   solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java 706f1ebe8b 
>   solr/test-framework/src/java/org/apache/solr/util/LogLevel.java 7542694441 
>   solr/test-framework/src/test-files/log4j.properties f6fedb6ea2 
>   solr/test-framework/src/test-files/log4j2.xml PRE-CREATION 
>   solr/test-framework/src/test/org/apache/solr/TestLogLevelAnnotations.java 
> 2ede874e50 
> 
> 
> Diff: https://reviews.apache.org/r/65888/diff/2/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Varun Thacker
> 
>

Reply via email to