Skip to content

MQTT: Throw Runtime Exception when manual ack fails#22117

Merged
davsclaus merged 3 commits intoapache:mainfrom
nkokitkar:nkokitkar/mqtt_expFix
Apr 27, 2026
Merged

MQTT: Throw Runtime Exception when manual ack fails#22117
davsclaus merged 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.

@nkokitkar nkokitkar requested review from Croway and gnodet April 3, 2026 23:26
@nkokitkar
Copy link
Copy Markdown
Contributor Author

@gnodet Can you please review this PR again? Thanks!

@davsclaus davsclaus force-pushed the nkokitkar/mqtt_expFix branch from ec82c09 to 62baa4c Compare April 17, 2026 12:51
@nkokitkar
Copy link
Copy Markdown
Contributor Author

@gnodet Can you please review this PR again? Thanks!

@gnodet Gentle ping for re-review

@nkokitkar
Copy link
Copy Markdown
Contributor Author

@Croway Is it possible to merge this PR with only 1 approval? Wanted to confirm as I don't see an option to merge this PR.

@davsclaus
Copy link
Copy Markdown
Contributor

@davsclaus
Copy link
Copy Markdown
Contributor

@apupier it seems CI job is installing mvnd 1.0.3 and later we use 1.0.5 so its not found

Prepare all required actions
Run ./.github/actions/install-mvnd
Run curl -fsSL -o mvnd.zip https://downloads.apache.org/maven/mvnd/$VERSION/maven-mvnd-$VERSION-$DISTRIBUTION.zip
Run curl -fsSL -o mvnd.zip.sha256 https://downloads.apache.org/maven/mvnd/$VERSION/maven-mvnd-$VERSION-$DISTRIBUTION.zip.sha256
Run echo "$(cat mvnd.zip.sha256) mvnd.zip" | sha256sum --check
mvnd.zip: OK
Run unzip mvnd.zip -d /tmp/
Archive: mvnd.zip
creating: /tmp/maven-mvnd-1.0.3-linux-amd64/
inflating: /tmp/maven-mvnd-1.0.3-linux-amd64/LICENSE.txt
inflating: /tmp/maven-mvnd-1.0.3-linux-amd64/NOTICE.txt
inflating: /tmp/maven-mvnd-1.0.3-linux-amd64/README.adoc
creating: /tmp/maven-mvnd-1.0.3-linux-amd64/bin/
inflating: /tmp/maven-mvnd-1.0.3-linux-amd64/bin/mvnd

...

/home/runner/work/camel/camel/./.github/actions/incremental-build/incremental-build.sh: line 748: /tmp/maven-mvnd-1.0.5-linux-amd64/bin/mvnd: No such file or directory

Maven build completed with exit code: 127

nkokitkar and others added 3 commits April 27, 2026 13:57
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>
@davsclaus davsclaus force-pushed the nkokitkar/mqtt_expFix branch from 62baa4c to 83e0c9d Compare April 27, 2026 11:57
@github-actions
Copy link
Copy Markdown
Contributor

🧪 CI tested the following changed modules:

  • components/camel-paho-mqtt5
  • components/camel-paho
All tested modules (9 modules)
  • Camel :: JBang :: MCP
  • Camel :: JBang :: Plugin :: Route Parser
  • Camel :: JBang :: Plugin :: TUI
  • Camel :: JBang :: Plugin :: Validate
  • Camel :: Launcher :: Container
  • Camel :: Paho
  • Camel :: Paho MQTT 5
  • Camel :: YAML DSL :: Validator
  • Camel :: YAML DSL :: Validator Maven Plugin

⚙️ View full build and test results

@davsclaus davsclaus merged commit 1165776 into apache:main Apr 27, 2026
6 checks passed
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.

5 participants