[AMQ-7426] Upgrade to log4j2 #662
Conversation
|
You'll definitely want to use something newer than 2.14.1 given the recently discovered severe vulnerability. |
jbonofre
force-pushed
the
AMQ-7426
branch
2 times, most recently
from
c5dc061
to
f76c09f
Compare
|
Runtime/assembly works fine. Now I have to fix some tests which are using log4j (logger/appender). |
jbonofre
force-pushed
the
AMQ-7426
branch
2 times, most recently
from
d7b6621
to
cf571ce
Compare
|
Test configs can/should use 'log4j2-test.properties' rather than 'log4j2.properties', it has a higher priority than regular files and makes it easier/quicker to distinguish the non-test config amongst the sea of files. |
|
There remain a lot of dependencies on log4j 1.x even after these changes. I'd suggest you should take a look at some of the AMQ 8472 related reload4j changes again and incorporating related aspects to address them (e.g perhaps also using the log4j2 api bridge for them if needed).
|
|
I will review the dependencies. Regarding log4j properties, it uses the same as before. So I guess the comment is not related to log4j2 update but more general comment. |
|
I'd say its a comment about the update since you are creating all the new files now, and I dont think log4j 1 had that functionality. |
|
Catcha. Thanks. I'm updating the PR. |
|
Still seeing a bunch of log4j 1.x deps, less than before but still several. Same number of slf4j-log4j12 deps. |
|
@gemmellr most of them are coming from commons-logging dependency in activeio. Should I exclude it ? |
|
I would say so, if the idea is to not use Log4J 1.x, then there should be no trace of it. Those bits should be using Log4J 2 (via the 1.2 bridge), or a less apealling alternative would be subbing in reload4j, but that doesnt really make sense given the context. |
|
Alright, I add the exclude. |
|
@gemmellr PR updated |
There was a problem hiding this comment.
Still some remaining log4j 1 instances (from zookeeper and pax stuff, as on the reload4j PR):
$ mvn dependency:tree|grep " log4j:log4j"
[INFO] | +- log4j:log4j:jar:1.2.17:provided
[INFO] | - log4j:log4j:jar:1.2.17:compile
[INFO] | +- log4j:log4j:jar:1.2.17:compile
[INFO] | | +- log4j:log4j:jar:1.2.17:compile
[INFO] | +- log4j:log4j:jar:1.2.17:compile
$ mvn dependency:tree|grep "slf4j-log4j12"
[INFO] | +- org.slf4j:slf4j-log4j12:jar:1.7.25:provided
[INFO] | +- org.slf4j:slf4j-log4j12:jar:1.7.25:compile
[INFO] | | +- org.slf4j:slf4j-log4j12:jar:1.7.25:compile
[INFO] | +- org.slf4j:slf4j-log4j12:jar:1.7.25:compile
assembly/pom.xml
Outdated
| </dependency> | ||
| <dependency> | ||
| <groupId>org.slf4j</groupId> | ||
| <artifactId>jcl-over-slf4j</artifactId> | ||
| <version>${slf4j-version}</version> |
There was a problem hiding this comment.
Redundant version declaration, it is (once again) managed.
This much should be expected given it was one of the main things in the reload4j PR, e.g 1f01a61#diff-9c5fb3d1b7e3b0f54bc5c4182965c4fe1f9023d449017cece3005d3f90e8e4d8R623-R656 and 1f01a61#diff-9c5fb3d1b7e3b0f54bc5c4182965c4fe1f9023d449017cece3005d3f90e8e4d8R491-R512 A likely difference in this case might be the addition of the log4j2 bridge for the 1.2 api. |
|
@gemmellr let me double check, I thought I did it already. |
|
Easy to check with a little grep :) |
|
No log4j1 dep anymore |
|
I don't see log4j1 anymore so that part looks good |
|
Yep, no log4j 1 and no slf4j-log4j12 mentions now. The mentions of log4j look to all be 2.17.1 now. |
|
I think this is ready to merge at this point if the tests pass (i don't think i see any other changes) |
|
Think it needs a rebase after the recent force push on main. The Jenkins build fell over, and prodding a restart it doesnt like trying to merge it. |
|
Rebased |
|
Last change breaks some tests, I'm fixing (probably due to dependency updates). I'm on it. |
|
All should be good now. I'm just waiting for Jenkins green light before merging. |
|
@jbonofre - We all good to merge?Looks like the checks passed |
No description provided.