Skip to content

MQTT: Throw Runtime Exception when manual ack fails#22117

Open
nkokitkar wants to merge 3 commits intoapache:mainfrom
nkokitkar:nkokitkar/mqtt_expFix
Open

MQTT: Throw Runtime Exception when manual ack fails#22117
nkokitkar wants to merge 3 commits intoapache:mainfrom
nkokitkar:nkokitkar/mqtt_expFix

Conversation

@nkokitkar
Copy link
Copy Markdown
Contributor

Description

Small fix to throw an exception when manual ack failed instead of a warning log.

Target

  • I checked that the commit is targeting the correct branch (Camel 4 uses the main branch)

Tracking

  • If this is a large change, bug fix, or code improvement, I checked there is a JIRA issue filed for the change (usually before you start working on it).

Apache Camel coding standards and style

  • I checked that each commit in the pull request has a meaningful subject line and body.
  • I have run mvn clean install -DskipTests locally from root folder and I have committed all auto-generated changes.

Copy link
Copy Markdown
Contributor

@gnodet gnodet left a comment

Choose a reason for hiding this comment

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

Review — Claude Code on behalf of Guillaume Nodet

Verdict: REQUEST CHANGES (recommend closing)

This PR has a fundamental design flaw that makes the change ineffective.

CRITICAL: RuntimeException is silently swallowed

The onComplete() callback is invoked by UnitOfWorkHelper.doneSynchronization(), which catches all exceptions:

// UnitOfWorkHelper.java
try {
    synchronization.onComplete(exchange);
} catch (Exception e) {
    // logged and swallowed
}

This means the RuntimeException("Exchange was not acknowledged") thrown by this PR is silently caught and discarded. The MQTT message is still NOT acknowledged (same behavior as before the change), but now there's a misleading exception in the logs with no actionable outcome.

The change provides zero functional benefit.

Other issues

  1. No tests — No unit or integration tests demonstrate or verify the intended behavior.
  2. No JIRA issue — A behavioral change in message acknowledgement should have a tracking issue.
  3. No documentation — No docs explaining when ACKNOWLEDGE_ALLOWED would be false or what the expected behavior should be.

Suggested alternative approaches

If the goal is to signal that acknowledgement was denied:

  • Log a warning directly (simplest — don't throw, just log)
  • Set the exchange as failed (exchange.setException(...)) before the synchronization callback
  • Use the onFailure() path instead of onComplete()
  • Use a custom error handler to handle the denial

The current approach of throwing from onComplete() is architecturally incompatible with how Camel processes synchronization callbacks.

@oscerd
Copy link
Copy Markdown
Contributor

oscerd commented Mar 20, 2026

If we change the behavior this require a JIRA isssue.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 2, 2026

🌟 Thank you for your contribution to the Apache Camel project! 🌟
🤖 CI automation will test this PR automatically.

🐫 Apache Camel Committers, please review the following items:

  • First-time contributors require MANUAL approval for the GitHub Actions to run
  • You can use the command /component-test (camel-)component-name1 (camel-)component-name2.. to request a test from the test bot although they are normally detected and executed by CI.
  • You can label PRs using build-all, build-dependents, skip-tests and test-dependents to fine-tune the checks executed by this PR.
  • Build and test logs are available in the summary page. Only Apache Camel committers have access to the summary.

⚠️ Be careful when sharing logs. Review their contents before sharing them publicly.

@Override
public void onFailure(Exchange exchange) {
LOG.error("Rollback due to error processing Exchange ID: {}", exchange.getExchangeId(),
LOG.debug("Rollback due to error processing Exchange ID: {}", exchange.getExchangeId(),
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.

why debug instead of error?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Reverted

Revert the log level in onFailure from debug back to error, as
this callback handles exchange processing failures which should
not be silently swallowed at debug level.

Addresses review comment from @Croway.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@nkokitkar nkokitkar requested review from Croway and gnodet April 3, 2026 23:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants