Skip to content

Fix KafkaAppender reporting error after successful retry - #4125

Merged
ramanathan1504 merged 4 commits into
apache:2.xfrom
SebTardif:fix/kafka-appender-retry-logic
May 20, 2026
Merged

ramanathan1504 merged 4 commits into
apache:2.xfrom
SebTardif:fix/kafka-appender-retry-logic

Conversation

@SebTardif

Copy link
Copy Markdown
Contributor

Fix KafkaAppender retry logic that always reports an error to the error handler even when a retry succeeds.

When retryCount is configured and the initial tryAppend() fails, the retry loop uses break to exit on success. However, break only exits the while loop; execution always reaches the error() call afterward. This causes spurious error notifications for transient Kafka failures that were successfully recovered by a retry.

This change replaces break with return so that a successful retry exits append() without reporting an error. Retry exceptions are now logged at DEBUG level for diagnostics instead of being silently discarded.

Also removes dead code in Builder.getRetryCount() where Integer.valueOf(int) was wrapped in a NumberFormatException catch that can never fire.

The bug was introduced in #315.

Checklist

  • Base your changes on 2.x branch if you are targeting Log4j 2; use main otherwise
  • ./mvnw verify succeeds (the build instructions)
  • Tests are provided

@ramanathan1504 ramanathan1504 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall — great fix for the break/return retry behavior, and thanks for adding DEBUG logging plus cleaning up getRetryCount().

One small suggestion commented to keep the diff minimal: retain the existing while loop shape and just apply the early return + DEBUG logging inside it.

SebTardif added 2 commits May 18, 2026 18:04
When retryCount is configured and the initial tryAppend() fails, the
retry loop uses break to exit on success. However, break only exits the
while loop and execution always reaches the error() call afterward. This
causes spurious error notifications for transient Kafka failures that
were successfully recovered by a retry.

Replace break with return so that a successful retry exits append()
without reporting an error. Retry exceptions are now logged at DEBUG
level for diagnostics instead of being silently discarded.

Also remove dead code in Builder.getRetryCount() where Integer.valueOf(int)
was wrapped in a NumberFormatException catch that can never fire.

The bug was introduced in apache#315.

Signed-off-by: Sebastien Tardif <[email protected]>
@SebTardif
SebTardif force-pushed the fix/kafka-appender-retry-logic branch from 42bc3fe to 2871852 Compare May 19, 2026 01:04
@ramanathan1504

Copy link
Copy Markdown
Contributor

Hi @SebTardif ,

Thank you so much for your patience and for contributing to this! We really appreciate the time and effort you've put into getting this KafkaAppender issue resolved. The code changes look good and the review is cleared!

Before we can finally merge this, there are just a few quick administrative things we need to address:

  • Avoid Force-Pushing during reviews: I noticed that the recent updates were force-pushed. For future PRs, please just push new commits normally rather than force-pushing. Force-pushing overwrites the Git history, which makes it very difficult for reviewers to track the differences between the old and new changes.
  • Commit Signatures: It looks like your commits are currently signed with a self-signed key. For security and verification purposes, we require commits to be signed off using a valid GPG key. Could you please set up a GPG key, re-sign your commits, and update the PR?
  • Add a Changelog Entry: Please add an entry to our changelog file for this fix. You can use this exact wording:

    "Fixed an issue where KafkaAppender incorrectly reported an error to the error handler despite a successful retry."

Next Steps:
Since you need to re-sign your previous commits and add the changelog, you will actually need to force-push one last time to update this PR. Once you've added the changelog and signed with a GPG key, go ahead and push it up, and we'll get this merged!

Thanks again for your great work on this!

@vy

vy commented May 19, 2026

Copy link
Copy Markdown
Member

Great guidance @ramanathan1504! 💯

  • Commit Signatures: It looks like your commits are currently signed with a self-signed key. For security and verification purposes, we require commits to be signed off using a valid GPG key.

@ramanathan1504, @SebTardif, we've dropped the requirement for commit signatures — see #3989. You can skip that chore.

Adopt reviewer suggestion to use try-with-resources CloseableThreadContext
instead of manual ThreadContext.put/clearMap.
@SebTardif

Copy link
Copy Markdown
Contributor Author

Thanks @ramanathan1504 for the review guidance and @vy for clarifying on signatures.

Adopted CloseableThreadContext as suggested. Pushed as a new commit this time (no force-push).

@ramanathan1504
ramanathan1504 enabled auto-merge (squash) May 19, 2026 18:51
@ramanathan1504

Copy link
Copy Markdown
Contributor

@SebTardif

Thanks for the update and for addressing the review comments.

I noticed the build failed due to Spotless formatting checks, so I pushed a small formatting-only fix to get the workflows green again. For future PRs, please make sure to run the Spotless checks locally before pushing to avoid CI failures.

Everything looks good now.

@ramanathan1504
ramanathan1504 merged commit 12380a6 into apache:2.x May 20, 2026
6 checks passed
@github-project-automation github-project-automation Bot moved this from Approved to Merged in Log4j pull request tracker May 20, 2026
@ramanathan1504 ramanathan1504 added this to the 2.26.1 milestone Jun 23, 2026
ramanathan1504 pushed a commit to ramanathan1504/logging-log4j2 that referenced this pull request Jul 9, 2026
* Fix KafkaAppender reporting error after successful retry

When retryCount is configured and the initial tryAppend() fails, the
retry loop uses break to exit on success. However, break only exits the
while loop and execution always reaches the error() call afterward. This
causes spurious error notifications for transient Kafka failures that
were successfully recovered by a retry.

Replace break with return so that a successful retry exits append()
without reporting an error. Retry exceptions are now logged at DEBUG
level for diagnostics instead of being silently discarded.

Also remove dead code in Builder.getRetryCount() where Integer.valueOf(int)
was wrapped in a NumberFormatException catch that can never fire.

The bug was introduced in apache#315.

Signed-off-by: Sebastien Tardif <[email protected]>

* Add changelog entry for KafkaAppender retry fix

Signed-off-by: Sebastien Tardif <[email protected]>

* Use CloseableThreadContext in testRetrySuccessDoesNotReportError

Adopt reviewer suggestion to use try-with-resources CloseableThreadContext
instead of manual ThreadContext.put/clearMap.

---------

Signed-off-by: Sebastien Tardif <[email protected]>
ramanathan1504 added a commit that referenced this pull request Jul 24, 2026
#4118)

* Fix NPE in ConfigurationScheduler by ensuring scheduledFuture is checked for null before accessing its methods; add test for race condition in scheduling.

* Restrict JUnit dependencies to below version 6.x to maintain compatib… (#4120)

* Restrict JUnit dependencies to below version 6.x to maintain compatibility with Java 8

* Update JUnit dependency restrictions for compatibility with Java 8 on 2.x

* Don't override `apache-rat-plugin` version (#4123)

Remove the version override for `apache-rat-plugin` so it falls back to the version provided by the ASF Parent POM.

Apache RAT `0.18`, for which Dependabot opened an update PR, contains a bug ([RAT-552](https://issues.apache.org/jira/browse/RAT-552)) that effectively disables the `excludeSubProjects` plugin option whenever `<excludes>` is present in the configuration.

Upgrading to `0.18` will require converting our `<excludes>` to `<inputExcludes>`, which depends on a new `logging-parent` release.

* Bump actions/stale from 9.1.0 to 10.2.0 in the dependencies group across 1 directory (#4121)

* Bump actions/stale in the dependencies group across 1 directory

Bumps the dependencies group with 1 update in the / directory: [actions/stale](https://github.com/actions/stale).


Updates `actions/stale` from 9.1.0 to 10.2.0
- [Release notes](https://github.com/actions/stale/releases)
- [Changelog](https://github.com/actions/stale/blob/main/CHANGELOG.md)
- [Commits](actions/stale@5bef64f...b5d41d4)

---
updated-dependencies:
- dependency-name: actions/stale
  dependency-version: 10.2.0
  dependency-type: direct:production
  update-type: version-update:semver-major
  dependency-group: dependencies
...

Signed-off-by: dependabot[bot] <[email protected]>

* Generate changelog entries for #4121

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* Fix KafkaAppender reporting error after successful retry (#4125)

* Fix KafkaAppender reporting error after successful retry

When retryCount is configured and the initial tryAppend() fails, the
retry loop uses break to exit on success. However, break only exits the
while loop and execution always reaches the error() call afterward. This
causes spurious error notifications for transient Kafka failures that
were successfully recovered by a retry.

Replace break with return so that a successful retry exits append()
without reporting an error. Retry exceptions are now logged at DEBUG
level for diagnostics instead of being silently discarded.

Also remove dead code in Builder.getRetryCount() where Integer.valueOf(int)
was wrapped in a NumberFormatException catch that can never fire.

The bug was introduced in #315.

Signed-off-by: Sebastien Tardif <[email protected]>

* Add changelog entry for KafkaAppender retry fix

Signed-off-by: Sebastien Tardif <[email protected]>

* Use CloseableThreadContext in testRetrySuccessDoesNotReportError

Adopt reviewer suggestion to use try-with-resources CloseableThreadContext
instead of manual ThreadContext.put/clearMap.

---------

Signed-off-by: Sebastien Tardif <[email protected]>

* Revamped `ListAppender` for thread-safety (#4111)

Co-authored-by: Björn Michael <[email protected]>
Co-authored-by: Volkan Yazıcı <[email protected]>

* Improve `CronExpression` tests to cover daylight saving and scheduling logic (#4081)

Co-authored-by: Piotr P. Karwasz <[email protected]>
Co-authored-by: Volkan Yazıcı <[email protected]>

* Fix `createOnDemand` for Rolling File Appender (#4072)

Co-authored-by: Piotr P. Karwasz <[email protected]>
Co-authored-by: Volkan Yazıcı <[email protected]>

* Add support for max compression delay in compression actions (#4071)

Co-authored-by: Volkan Yazıcı <[email protected]>

* Improve logging for `LinkageError` scenarios involving the LMAX Disruptor library (#4124)

Co-authored-by: Volkan Yazıcı <[email protected]>

* Fix changelog issue and PR references for Disruptor initialization error logging (#2250, #4124, #4134)

* Fix encoding of `MSGID` and `SD-ID` fields of `StructuredDataMessage` to XML (#4136)

* Fix stack trace rendering for exceptions with identity malfunction (#4133)

* Fix circular reference detection for exceptions with colliding equals/hashCode implementations

* Add changelog entry for circular reference detection fix with colliding equals/hashCode exceptions

* Add changelog entry for circular reference detection fix with colliding equals/hashCode exceptions

* Add test for ThrowableProxy serialization with colliding equals/hashCode implementations

* Refactor ThrowableProxy serialization test to use modern Java I/O classes

* Use `TestFriendlyException` to exercise the malfunction

* Update changelog

* Improve comments

* Remove redundant change

---------

Co-authored-by: Volkan Yazıcı <[email protected]>

* Remove the `patternFlags` attribute from `RegexFilter` (#4119)

Co-authored-by: Jeff Thomas <[email protected]>
Co-authored-by: Volkan Yazıcı <[email protected]>

* Fix `DatePatternConverter` locale parsing when timezone is omitted (#4130)

* Harden `readObject(ObjectInputStream)` method argument checks (#4098)

Signed-off-by: SunWeb3Sec <[email protected]>
Co-authored-by: Volkan Yazıcı <[email protected]>

* Fix resource leaks in `ConfigurationSource` when loading via URL (#4127)

Signed-off-by: Sebastien Tardif <[email protected]>
Co-authored-by: Ramanathan <[email protected]>
Co-authored-by: Piotr P. Karwasz <[email protected]>
Co-authored-by: Volkan Yazıcı <[email protected]>

* Adopt the Dependabot changelog 'draft trick' (#4148)

Applies the consumer-side changes from logging-parent PR #476:

* `build.yaml` and `codeql-analysis.yaml` now subscribe to the
  `ready_for_review` pull request type, so required checks re-run when a
  Dependabot PR is taken out of draft.
* `process-dependabot.yaml` drops the `RECURSIVE_TOKEN` PAT secret, which
  is no longer needed now that the reusable workflow parks the PR in
  draft mode instead of pushing with a privileged token.

Assisted-By: Claude Opus 4.8 (1M context) <[email protected]>

* Add Dependabot config for the 2.26.x maintenance branch (#4149)

Configure a Maven update entry targeting the `2.26.x` branch that only
proposes patch-level upgrades. All dependencies are bundled into a single
`Maven patch updates` group with a 7-day cooldown.

* Fix handling of non-finite numbers while encoding `MapMessage` to JSON (#4163)

* Remove `/log4j-slf4j-impl` Dependabot execution (#4151)

Instead of correcting the target branch, we can safely
remove it: the only dependency version defined in
`log4j-slf4j-impl` is SLF4J 1.x, which is highly unlikely
to see any new releases.

* Upgrade Maven and Maven Wrapper versions to `3.9.16` and `3.3.4`, respectively (#4167)

* Adds `maxRandomDelay` configure to Rolling Appenders (#4165)

* Rework random delay for Rolling Appender action chain

* Stabilize `RollingAppenderDirectCronTest`

* Merge `2.26.1` to `2.x` (#4169)

* Add `2.25.5` release notes (#4174)

* Add native tracing fields to LogEvent to eliminate async MDC overhead (#4171)

* Add tracing fields to RingBufferLogEvent and related classes

* Add W3C trace context support with pattern converters for trace ID, span ID, and trace flags

* Enhance TraceContextProviderService for exception safety and simplify trace ID retrieval

* Add tests for tracing fields serialization and pattern converters

* Refactor tracing metadata documentation and improve code comments for clarity

* Add native W3C tracing fields to LogEvent and introduce TraceContextProvider SPI

* Add benchmark for ContextDataProvider tracing approach

* Enhance TraceContextProviderService to handle SecurityManager restrictions gracefully

* Fix NPE in ConfigurationScheduler by ensuring proper reset of scheduled futures

* Fix NPE in ConfigurationScheduler for frequent cron schedules

---------

Signed-off-by: dependabot[bot] <[email protected]>
Signed-off-by: Sebastien Tardif <[email protected]>
Signed-off-by: SunWeb3Sec <[email protected]>
Co-authored-by: Piotr P. Karwasz <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Sebastien Tardif <[email protected]>
Co-authored-by: Björn Michael <[email protected]>
Co-authored-by: Volkan Yazıcı <[email protected]>
Co-authored-by: Volkan Yazıcı <[email protected]>
Co-authored-by: Jeff Thomas <[email protected]>
Co-authored-by: SunWeb3Sec <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants