-
Notifications
You must be signed in to change notification settings - Fork 70
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Revert "[pom] log4j: bump to 2.13.2." #311
Conversation
This reverts commit 10e0610.
@@ -41,7 +41,7 @@ | |||
<guava.version>27.1-android</guava.version> <!-- From the docs: If you need support for JDK 1.7 or Android, use the Android flavor. --> | |||
<java-dogstatsd-client.version>2.9.0</java-dogstatsd-client.version> | |||
<jcommander.version>1.35</jcommander.version> | |||
<log4j.version>2.13.2</log4j.version> | |||
<log4j.version>2.12.1</log4j.version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A comment on this line would be good so the change doesn't happen again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, addressed 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than @devinsba comment, this looks good and sticking on 2.12.1
should not be harmful/dangerous given the set of features we use on JMXFetch.
This reverts commit 10e0610. Reason: log4j 2.13 drops Java 7 support
Reverts #309
log4j
only supports Java 1.8 and up since2.13
. We want to still support 1.7 for now, so we should go back to2.12.1
.(fwiw, this is mentioned in
log4j
's main docs page but not in its changelog...)We do have java 1.7 tests on Travis, not sure why they didn't catch this.
I confirmed this PR makes JMXFetch work again with Java 1.7.