You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by "luigidemasi (via GitHub)" <gi...@apache.org> on 2024/03/07 09:34:50 UTC
[PR] CAMEL-20521: camel-amqp - AMQP publisher application is losing messages with local JMS transaction enabled [camel]
luigidemasi opened a new pull request, #13403:
URL: https://github.com/apache/camel/pull/13403
(no comment)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-20521: camel-amqp - AMQP publisher application is losing messages with local JMS transaction enabled [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on code in PR #13403:
URL: https://github.com/apache/camel/pull/13403#discussion_r1518811060
##########
components/camel-amqp/src/main/java/org/apache/camel/component/amqp/AMQPConfiguration.java:
##########
@@ -51,4 +64,28 @@ public void setIncludeAmqpAnnotations(boolean includeAmqpAnnotations) {
this.includeAmqpAnnotations = includeAmqpAnnotations;
}
+ public static class CamelAmqpTemplate extends CamelJmsTemplate {
+
+ public CamelAmqpTemplate(AMQPConfiguration config, ConnectionFactory connectionFactory) {
+ super(config, connectionFactory);
+ }
+
+ @Override
+ protected void commitIfNecessary(Session session) throws JMSException {
+ //# CAMEL-20521 - we can't use JmsUtils.commitIfNecessary because it catches
+ // jakarta.jms.IllegalStateException that can be thrown if the session is closed
+
+ // Check commit - avoid commit call within a JTA transaction.
+ if (session.getTransacted() && isSessionLocallyTransacted(session)) {
+ // Transacted session created by this template -> commit.
+ Assert.notNull(session, "Session must not be null");
Review Comment:
This assert seems not to make sense as you will have NPE above
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-20521: camel-amqp - AMQP publisher application is losing messages with local JMS transaction enabled [camel]
Posted by "zhfeng (via GitHub)" <gi...@apache.org>.
zhfeng commented on PR #13403:
URL: https://github.com/apache/camel/pull/13403#issuecomment-1986834934
/component-test camel-amqp
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-20521: camel-amqp - AMQP publisher application is losing messages with local JMS transaction enabled [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on code in PR #13403:
URL: https://github.com/apache/camel/pull/13403#discussion_r1515867926
##########
components/camel-jms/src/main/java/org/apache/camel/component/jms/JmsConfiguration.java:
##########
@@ -620,7 +620,13 @@ private Object doSendToDestination(
// Check commit - avoid commit call within a JTA transaction.
if (session.getTransacted() && isSessionLocallyTransacted(session)) {
// Transacted session created by this template -> commit.
- JmsUtils.commitIfNecessary(session);
+ Assert.notNull(session, "Session must not be null");
+ try {
+ session.commit();
Review Comment:
Why are you not calling JmsUtils.commitIfNessasary ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-20521: camel-amqp - AMQP publisher application is losing messages with local JMS transaction enabled [camel]
Posted by "luigidemasi (via GitHub)" <gi...@apache.org>.
luigidemasi commented on PR #13403:
URL: https://github.com/apache/camel/pull/13403#issuecomment-1988266519
@davsclaus @zhfeng I ran my test this morning and I was able to reproduce the issue also with plain jms component. I'll continue to run tests but it seems that the issue is not limited to AMQP/Qpid unfortunately...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-20521: camel-amqp - AMQP publisher application is losing messages with local JMS transaction enabled [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on code in PR #13403:
URL: https://github.com/apache/camel/pull/13403#discussion_r1515896873
##########
components/camel-jms/src/main/java/org/apache/camel/component/jms/JmsConfiguration.java:
##########
@@ -620,7 +620,13 @@ private Object doSendToDestination(
// Check commit - avoid commit call within a JTA transaction.
if (session.getTransacted() && isSessionLocallyTransacted(session)) {
// Transacted session created by this template -> commit.
- JmsUtils.commitIfNecessary(session);
+ Assert.notNull(session, "Session must not be null");
+ try {
+ session.commit();
Review Comment:
No camel-jms is for JMS and should be as-is that is battle tested with spring-jms and all those kind of JMS brokers.
This ticket is about AMQP which is not JMS.
If there is a special need for camel-amqp in AMQP mode, then that must be done specially in camel-amqp and not in general camel-jms.
Spring likely dont care about your AMQP as they use rabbitmq and its AMQP version.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-20521: camel-amqp - AMQP publisher application is losing messages with local JMS transaction enabled [camel]
Posted by "zhfeng (via GitHub)" <gi...@apache.org>.
zhfeng commented on code in PR #13403:
URL: https://github.com/apache/camel/pull/13403#discussion_r1527672357
##########
catalog/camel-catalog/src/generated/resources/org/apache/camel/catalog/components/langchain-embeddings.json:
##########
Review Comment:
Not sure why it is here but I think all of the `langchain-embeddings` changes should be excluded from this commit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-20521: camel-amqp - AMQP publisher application is losing messages with local JMS transaction enabled [camel]
Posted by "luigidemasi (via GitHub)" <gi...@apache.org>.
luigidemasi commented on PR #13403:
URL: https://github.com/apache/camel/pull/13403#issuecomment-1997168742
> Given the scope and criticality of this (as losing messages is truly a bad thing), I think it would be important to consider adding actual integration tests, instead of relying on mocks. This is particularly important if this can happen in other circumstances / other brokers, IMHO
@orpiske The issue is not limited to the amqp component, I can reproduce it with plain camel-jms too, I added a test that reproduces the issue using a real Artemis broker running in a docker container.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-20521: camel-amqp - AMQP publisher application is losing messages with local JMS transaction enabled [camel]
Posted by "luigidemasi (via GitHub)" <gi...@apache.org>.
luigidemasi commented on code in PR #13403:
URL: https://github.com/apache/camel/pull/13403#discussion_r1515874734
##########
components/camel-jms/src/main/java/org/apache/camel/component/jms/JmsConfiguration.java:
##########
@@ -620,7 +620,13 @@ private Object doSendToDestination(
// Check commit - avoid commit call within a JTA transaction.
if (session.getTransacted() && isSessionLocallyTransacted(session)) {
// Transacted session created by this template -> commit.
- JmsUtils.commitIfNecessary(session);
+ Assert.notNull(session, "Session must not be null");
+ try {
+ session.commit();
Review Comment:
@davsclaus take a look at jira description, it's all there.
https://issues.apache.org/jira/browse/CAMEL-20521
I'll probably open a PR on spring as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-20521: camel-amqp - AMQP publisher application is losing messages with local JMS transaction enabled [camel]
Posted by "orpiske (via GitHub)" <gi...@apache.org>.
orpiske commented on code in PR #13403:
URL: https://github.com/apache/camel/pull/13403#discussion_r1516110769
##########
components/camel-jms/src/main/java/org/apache/camel/component/jms/JmsConfiguration.java:
##########
@@ -620,7 +620,13 @@ private Object doSendToDestination(
// Check commit - avoid commit call within a JTA transaction.
if (session.getTransacted() && isSessionLocallyTransacted(session)) {
// Transacted session created by this template -> commit.
- JmsUtils.commitIfNecessary(session);
+ Assert.notNull(session, "Session must not be null");
+ try {
+ session.commit();
Review Comment:
IMHO, a more appropriate fix for this would involve creating an specific JMS template for AMQP.
At a quick glace, it doesn't seem like the code easily allows this, so probably would involve some refactoring on the process.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-20521: camel-amqp - AMQP publisher application is losing messages with local JMS transaction enabled [camel]
Posted by "luigidemasi (via GitHub)" <gi...@apache.org>.
luigidemasi commented on PR #13403:
URL: https://github.com/apache/camel/pull/13403#issuecomment-2053638409
> spring upgrade here: #13778
Updated the PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-20521: camel-amqp - AMQP publisher application is losing messages with local JMS transaction enabled [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on PR #13403:
URL: https://github.com/apache/camel/pull/13403#issuecomment-2006335918
org.apache.camel.component.jms.JmsDeliveryDelayTest.testInOnlyWithDelay | 5.123s | FAILURE
-- | -- | --
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-20521: camel-amqp - AMQP publisher application is losing messages with local JMS transaction enabled [camel]
Posted by "luigidemasi (via GitHub)" <gi...@apache.org>.
luigidemasi commented on code in PR #13403:
URL: https://github.com/apache/camel/pull/13403#discussion_r1516174428
##########
components/camel-jms/src/main/java/org/apache/camel/component/jms/JmsConfiguration.java:
##########
@@ -620,7 +620,13 @@ private Object doSendToDestination(
// Check commit - avoid commit call within a JTA transaction.
if (session.getTransacted() && isSessionLocallyTransacted(session)) {
// Transacted session created by this template -> commit.
- JmsUtils.commitIfNecessary(session);
+ Assert.notNull(session, "Session must not be null");
+ try {
+ session.commit();
Review Comment:
Ok, I'll try to reproduce the problem with plain jms component, if it's only a AMQP/QPid problem, I'll create a JmsTemplate for AMQP.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-20521: camel-amqp - AMQP publisher application is losing messages with local JMS transaction enabled [camel]
Posted by "luigidemasi (via GitHub)" <gi...@apache.org>.
luigidemasi commented on PR #13403:
URL: https://github.com/apache/camel/pull/13403#issuecomment-2004215687
I also opened an issue on spring that has already been fixed:
https://github.com/spring-projects/spring-framework/issues/32473
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-20521: camel-amqp - AMQP publisher application is losing messages with local JMS transaction enabled [camel]
Posted by "zhfeng (via GitHub)" <gi...@apache.org>.
zhfeng commented on PR #13403:
URL: https://github.com/apache/camel/pull/13403#issuecomment-1988273497
@luigidemasi is it possible to share the reproducer?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-20521: camel-amqp - AMQP publisher application is losing messages with local JMS transaction enabled [camel]
Posted by "orpiske (via GitHub)" <gi...@apache.org>.
orpiske commented on PR #13403:
URL: https://github.com/apache/camel/pull/13403#issuecomment-1989448398
> @davsclaus @zhfeng @orpiske I ran my test this morning and I was able to reproduce the issue also with plain jms component and Apache Artemis 2.28.0 Broker. I'll continue to run tests but it seems that the issue is not limited to AMQP/Qpid unfortunately...
Given the scope and criticality of this (as losing messages is truly a bad thing), I think it would be important to consider adding actual integration tests, instead of relying on mocks. This is particularly important if this can happen in other circumstances / other brokers, IMHO
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-20521: camel-amqp - AMQP publisher application is losing messages with local JMS transaction enabled [camel]
Posted by "luigidemasi (via GitHub)" <gi...@apache.org>.
luigidemasi commented on PR #13403:
URL: https://github.com/apache/camel/pull/13403#issuecomment-2051434294
The new spring version, 6.1.6, that contains the fix for this issue, has been released. Should I close this issue or just modify the commit removing everything except tests?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-20521: camel-amqp - AMQP publisher application is losing messages with local JMS transaction enabled [camel]
Posted by "zhfeng (via GitHub)" <gi...@apache.org>.
zhfeng commented on code in PR #13403:
URL: https://github.com/apache/camel/pull/13403#discussion_r1518442503
##########
components/camel-amqp/src/main/java/org/apache/camel/component/amqp/AMQPConfiguration.java:
##########
@@ -51,4 +64,28 @@ public void setIncludeAmqpAnnotations(boolean includeAmqpAnnotations) {
this.includeAmqpAnnotations = includeAmqpAnnotations;
}
+ public static class CamelAmqpTemplate extends CamelJmsTemplate {
+
+ public CamelAmqpTemplate(AMQPConfiguration config, ConnectionFactory connectionFactory) {
+ super(config, connectionFactory);
+ }
+
+ @Override
+ protected void commitIfNecessary(Session session) throws JMSException {
+ //# CAMEL-20521 - we can't use JmsUtils.commitIfNecessary because it catches
+ // jakarta.jms.IllegalStateException that can be thrown if the session is closed
Review Comment:
I'm not sure why this can fix CAMEL-20521 when session is closed during the commit? It will lead to the re-connecting?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-20521: camel-amqp - AMQP publisher application is losing messages with local JMS transaction enabled [camel]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13403:
URL: https://github.com/apache/camel/pull/13403#issuecomment-1986834996
:robot: The Apache Camel test robot will run the tests for you :+1:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-20521: camel-amqp - AMQP publisher application is losing messages with local JMS transaction enabled [camel]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13403:
URL: https://github.com/apache/camel/pull/13403#issuecomment-1983101242
:star2: Thank you for your contribution to the Apache Camel project! :star2:
:robot: CI automation will test this PR automatically.
:camel: 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.
* 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](https://camel.apache.org/community/team/#committers) have access to the summary.
* :warning: Be careful when sharing logs. Review their contents before sharing them publicly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-20521: camel-amqp - AMQP publisher application is losing messages with local JMS transaction enabled [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus merged PR #13403:
URL: https://github.com/apache/camel/pull/13403
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-20521: camel-amqp - AMQP publisher application is losing messages with local JMS transaction enabled [camel]
Posted by "luigidemasi (via GitHub)" <gi...@apache.org>.
luigidemasi commented on code in PR #13403:
URL: https://github.com/apache/camel/pull/13403#discussion_r1528100793
##########
catalog/camel-catalog/src/generated/resources/org/apache/camel/catalog/components/langchain-embeddings.json:
##########
Review Comment:
Thanks Amos, done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-20521: camel-amqp - AMQP publisher application is losing messages with local JMS transaction enabled [camel]
Posted by "zhfeng (via GitHub)" <gi...@apache.org>.
zhfeng commented on code in PR #13403:
URL: https://github.com/apache/camel/pull/13403#discussion_r1518557066
##########
components/camel-amqp/src/main/java/org/apache/camel/component/amqp/AMQPConfiguration.java:
##########
@@ -51,4 +64,28 @@ public void setIncludeAmqpAnnotations(boolean includeAmqpAnnotations) {
this.includeAmqpAnnotations = includeAmqpAnnotations;
}
+ public static class CamelAmqpTemplate extends CamelJmsTemplate {
+
+ public CamelAmqpTemplate(AMQPConfiguration config, ConnectionFactory connectionFactory) {
+ super(config, connectionFactory);
+ }
+
+ @Override
+ protected void commitIfNecessary(Session session) throws JMSException {
+ //# CAMEL-20521 - we can't use JmsUtils.commitIfNecessary because it catches
+ // jakarta.jms.IllegalStateException that can be thrown if the session is closed
Review Comment:
OK, thanks for the explaination!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-20521: camel-amqp - AMQP publisher application is losing messages with local JMS transaction enabled [camel]
Posted by "luigidemasi (via GitHub)" <gi...@apache.org>.
luigidemasi commented on code in PR #13403:
URL: https://github.com/apache/camel/pull/13403#discussion_r1518555101
##########
components/camel-amqp/src/main/java/org/apache/camel/component/amqp/AMQPConfiguration.java:
##########
@@ -51,4 +64,28 @@ public void setIncludeAmqpAnnotations(boolean includeAmqpAnnotations) {
this.includeAmqpAnnotations = includeAmqpAnnotations;
}
+ public static class CamelAmqpTemplate extends CamelJmsTemplate {
+
+ public CamelAmqpTemplate(AMQPConfiguration config, ConnectionFactory connectionFactory) {
+ super(config, connectionFactory);
+ }
+
+ @Override
+ protected void commitIfNecessary(Session session) throws JMSException {
+ //# CAMEL-20521 - we can't use JmsUtils.commitIfNecessary because it catches
+ // jakarta.jms.IllegalStateException that can be thrown if the session is closed
Review Comment:
It leads to a redelivery if configured, this fix allows the exception to bubble up and can be handled by the error handler.
At the moment the exception is stopped at `org.springframework.jms.support.JmsUtils.commitIfNecessary`, not even logged at trace level, so the camel route continue on the normal flow, like the commit had been successful.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-20521: camel-amqp - AMQP publisher application is losing messages with local JMS transaction enabled [camel]
Posted by "luigidemasi (via GitHub)" <gi...@apache.org>.
luigidemasi commented on code in PR #13403:
URL: https://github.com/apache/camel/pull/13403#discussion_r1518555101
##########
components/camel-amqp/src/main/java/org/apache/camel/component/amqp/AMQPConfiguration.java:
##########
@@ -51,4 +64,28 @@ public void setIncludeAmqpAnnotations(boolean includeAmqpAnnotations) {
this.includeAmqpAnnotations = includeAmqpAnnotations;
}
+ public static class CamelAmqpTemplate extends CamelJmsTemplate {
+
+ public CamelAmqpTemplate(AMQPConfiguration config, ConnectionFactory connectionFactory) {
+ super(config, connectionFactory);
+ }
+
+ @Override
+ protected void commitIfNecessary(Session session) throws JMSException {
+ //# CAMEL-20521 - we can't use JmsUtils.commitIfNecessary because it catches
+ // jakarta.jms.IllegalStateException that can be thrown if the session is closed
Review Comment:
It leads to a redelivery if configured, this fix allows the exception to bubble up and can be handled by the error handler.
At the moment the exception is stopped at `org.springframework.jms.support.JmsUtils.commitIfNecessary`, not even logged at trace level, so the camel route continue on the normal flow, like the commit has ended successfully.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-20521: camel-amqp - AMQP publisher application is losing messages with local JMS transaction enabled [camel]
Posted by "orpiske (via GitHub)" <gi...@apache.org>.
orpiske commented on PR #13403:
URL: https://github.com/apache/camel/pull/13403#issuecomment-1999193426
> > Given the scope and criticality of this (as losing messages is truly a bad thing), I think it would be important to consider adding actual integration tests, instead of relying on mocks. This is particularly important if this can happen in other circumstances / other brokers, IMHO
>
> @orpiske The issue is not limited to the amqp component, I can reproduce it with plain camel-jms too, I added a test that reproduces the issue using a real Artemis broker running in a docker container.
Awesome, thank you!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-20521: camel-amqp - AMQP publisher application is losing messages with local JMS transaction enabled [camel]
Posted by "luigidemasi (via GitHub)" <gi...@apache.org>.
luigidemasi commented on code in PR #13403:
URL: https://github.com/apache/camel/pull/13403#discussion_r1518559799
##########
components/camel-jms/src/main/java/org/apache/camel/component/jms/JmsConfiguration.java:
##########
@@ -620,7 +620,13 @@ private Object doSendToDestination(
// Check commit - avoid commit call within a JTA transaction.
if (session.getTransacted() && isSessionLocallyTransacted(session)) {
// Transacted session created by this template -> commit.
- JmsUtils.commitIfNecessary(session);
+ Assert.notNull(session, "Session must not be null");
+ try {
+ session.commit();
Review Comment:
@davsclaus @orpiske, not able to reproduce with plain jms, it seems that this happens only with amqp/Qpid. I moved the changes to camel-amqp component.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-20521: camel-amqp - AMQP publisher application is losing messages with local JMS transaction enabled [camel]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13403:
URL: https://github.com/apache/camel/pull/13403#issuecomment-1986829792
:robot: The Apache Camel test robot will run the tests for you :+1:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-20521: camel-amqp - AMQP publisher application is losing messages with local JMS transaction enabled [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on PR #13403:
URL: https://github.com/apache/camel/pull/13403#issuecomment-1987174599
Can you update this PR as
```
diff --git a/catalog/camel-catalog/src/generated/resources/org/apache/camel/catalog/releases/camel-releases.json b/catalog/camel-catalog/src/generated/resources/org/apache/camel/catalog/releases/camel-releases.json
index 7f20c95b7c3..b32e2ae444c 100644
--- a/catalog/camel-catalog/src/generated/resources/org/apache/camel/catalog/releases/camel-releases.json
+++ b/catalog/camel-catalog/src/generated/resources/org/apache/camel/catalog/releases/camel-releases.json
@@ -859,6 +859,7 @@
{
"version": "4.4.0",
"date": "[20](https://github.com/apache/camel/actions/runs/8210131412/job/22481316768?pr=13403#step:8:21)24-02-17",
+ "eol": "2025-02-17",
"kind": "lts",
"jdk": "17,[21](https://github.com/apache/camel/actions/runs/8210131412/job/22481316768?pr=13403#step:8:22)"
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-20521: camel-amqp - AMQP publisher application is losing messages with local JMS transaction enabled [camel]
Posted by "zhfeng (via GitHub)" <gi...@apache.org>.
zhfeng commented on PR #13403:
URL: https://github.com/apache/camel/pull/13403#issuecomment-1986829717
/component-test camel-jms camel-amqp
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-20521: camel-amqp - AMQP publisher application is losing messages with local JMS transaction enabled [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on PR #13403:
URL: https://github.com/apache/camel/pull/13403#issuecomment-2051831574
@luigidemasi it would be great if we could have the tests (great work). And then upgrade spring (that can be in another PR)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-20521: camel-amqp - AMQP publisher application is losing messages with local JMS transaction enabled [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on PR #13403:
URL: https://github.com/apache/camel/pull/13403#issuecomment-2051949389
spring upgrade here: https://github.com/apache/camel/pull/13778
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org