Skip to content

Revert "Bump log4j2 from 2.21.0 to 2.25.1"#19430

Merged
andrross merged 1 commit intoopensearch-project:mainfrom
andrross:revert-log4j
Sep 26, 2025
Merged

Revert "Bump log4j2 from 2.21.0 to 2.25.1"#19430
andrross merged 1 commit intoopensearch-project:mainfrom
andrross:revert-log4j

Conversation

@andrross
Copy link
Copy Markdown
Member

@andrross andrross commented Sep 26, 2025

This reverts commit b9c5bc7 from #19184

The log4j update has introduced flakiness in a test (#19325) for an unknown reason. It also has downstream impact on plugins due to the addition of error prone annotations as an API dependency in :server. I don't understand the changes to OpenSearchTestCase, or why we now have a flaky test, so I'm putting out this revert PR. Unfortunately if we do commit this revert then plugins that have already updated to deal with the error prone annotation change will have to make another change.

I'm open to other suggestions to fix the test. @cwperks was able to stabilize the tests with #19426, but neither of us understand why that would be necessary.

Check List

  • Functionality includes testing.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

This reverts commit b9c5bc7.

Signed-off-by: Andrew Ross <andrross@amazon.com>
@cwperks
Copy link
Copy Markdown
Member

cwperks commented Sep 26, 2025

Related bug: apache/logging-log4j2#3940

@cwperks
Copy link
Copy Markdown
Member

cwperks commented Sep 26, 2025

When inspecting the value of statusToString I get the following stack trace:

  1> Throwable:
  1> java.lang.ArrayIndexOutOfBoundsException: Index 7 out of bounds for length 7
  1>    at org.apache.logging.log4j.core.pattern.ThrowableStackTraceRenderer.renderStackTraceElements(ThrowableStackTraceRenderer.java:159)
  1>    at org.apache.logging.log4j.core.pattern.ThrowableStackTraceRenderer.renderThrowable(ThrowableStackTraceRenderer.java:100)
  1>    at org.apache.logging.log4j.core.pattern.ThrowableStackTraceRenderer.renderCause(ThrowableStackTraceRenderer.java:136)
  1>    at org.apache.logging.log4j.core.pattern.ThrowableStackTraceRenderer.renderThrowable(ThrowableStackTraceRenderer.java:103)
  1>    at org.apache.logging.log4j.core.pattern.ThrowableStackTraceRenderer.renderThrowable(ThrowableStackTraceRenderer.java:77)
  1>    at org.apache.logging.log4j.core.pattern.ThrowableStackTraceRenderer.renderThrowable(ThrowableStackTraceRenderer.java:56)
  1>    at org.apache.logging.log4j.core.pattern.ThrowablePatternConverter.format(ThrowablePatternConverter.java:130)
  1>    at org.apache.logging.log4j.core.pattern.PatternFormatter.format(PatternFormatter.java:44)
  1>    at org.apache.logging.log4j.core.layout.PatternLayout$PatternFormatterPatternSerializer.toSerializable(PatternLayout.java:396)
  1>    at org.apache.logging.log4j.core.layout.PatternLayout.toText(PatternLayout.java:251)
  1>    at org.apache.logging.log4j.core.layout.PatternLayout.encode(PatternLayout.java:237)
  1>    at org.apache.logging.log4j.core.layout.PatternLayout.encode(PatternLayout.java:57)
  1>    at org.apache.logging.log4j.core.appender.AbstractOutputStreamAppender.directEncodeEvent(AbstractOutputStreamAppender.java:227)
  1>    at org.apache.logging.log4j.core.appender.AbstractOutputStreamAppender.tryAppend(AbstractOutputStreamAppender.java:220)
  1>    at org.apache.logging.log4j.core.appender.AbstractOutputStreamAppender.append(AbstractOutputStreamAppender.java:211)
  1>    at org.apache.logging.log4j.core.config.AppenderControl.tryCallAppender(AppenderControl.java:160)
  1>    at org.apache.logging.log4j.core.config.AppenderControl.callAppender0(AppenderControl.java:133)
  1>    at org.apache.logging.log4j.core.config.AppenderControl.callAppenderPreventRecursion(AppenderControl.java:124)
  1>    at org.apache.logging.log4j.core.config.AppenderControl.callAppender(AppenderControl.java:88)
  1>    at org.apache.logging.log4j.core.config.LoggerConfig.callAppenders(LoggerConfig.java:711)
  1>    at org.apache.logging.log4j.core.config.LoggerConfig.processLogEvent(LoggerConfig.java:669)
  1>    at org.apache.logging.log4j.core.config.LoggerConfig.log(LoggerConfig.java:645)
  1>    at org.apache.logging.log4j.core.config.LoggerConfig.log(LoggerConfig.java:589)
  1>    at org.apache.logging.log4j.core.config.AwaitCompletionReliabilityStrategy.log(AwaitCompletionReliabilityStrategy.java:92)
  1>    at org.apache.logging.log4j.core.Logger.log(Logger.java:187)
  1>    at org.apache.logging.log4j.spi.AbstractLogger.tryLogMessage(AbstractLogger.java:2970)
  1>    at org.apache.logging.log4j.spi.AbstractLogger.logMessageTrackRecursion(AbstractLogger.java:2922)
  1>    at org.apache.logging.log4j.spi.AbstractLogger.logMessageSafely(AbstractLogger.java:2904)
  1>    at org.apache.logging.log4j.spi.AbstractLogger.logMessage(AbstractLogger.java:2648)
  1>    at org.apache.logging.log4j.spi.AbstractLogger.logIfEnabled(AbstractLogger.java:2587)
  1>    at org.apache.logging.log4j.spi.AbstractLogger.error(AbstractLogger.java:824)

Test case run:

./gradlew ':server:test' --tests 'org.opensearch.gateway.remote.RemoteClusterStateServiceTests.testReadClusterStateInParallel_ExceptionDuringRead' -Dtests.iters=10 -i

@cwperks
Copy link
Copy Markdown
Member

cwperks commented Sep 26, 2025

I see a few calls to setStackTrace in the core, could it be related?

@github-actions
Copy link
Copy Markdown
Contributor

✅ Gradle check result for 58cd5c6: SUCCESS

@codecov
Copy link
Copy Markdown

codecov bot commented Sep 26, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.98%. Comparing base (a19434c) to head (58cd5c6).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main   #19430      +/-   ##
============================================
+ Coverage     72.89%   72.98%   +0.09%     
- Complexity    69911    69967      +56     
============================================
  Files          5676     5676              
  Lines        321142   321142              
  Branches      46429    46429              
============================================
+ Hits         234085   234398     +313     
+ Misses        68144    67822     -322     
- Partials      18913    18922       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@msfroh
Copy link
Copy Markdown
Contributor

msfroh commented Sep 26, 2025

Reading the log4j issue that @cwperks linked, it sounds like this could potentially have production impact, at least in terms of either increasing logging or obfuscating logging. Throwing another exception while trying to log an exception feels pretty bad.

I'm onboard with reverting.

@andrross andrross merged commit a08700c into opensearch-project:main Sep 26, 2025
53 checks passed
@andrross andrross deleted the revert-log4j branch September 26, 2025 16:02
@navneet1v
Copy link
Copy Markdown
Contributor

[nit-pick]
@andrross next time when we get error_prone added as a dependency can we make sure that the version we pick is present in google guava. This will avoid removing the error_prone dependencies from plugins who also depends on guava

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants