You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by GitBox <gi...@apache.org> on 2020/10/07 13:23:32 UTC
[GitHub] [camel] JSchoreels opened a new pull request #4384: AutomaticRecovery from RabbitMQ Connection Factory doesn't recover from everything
JSchoreels opened a new pull request #4384:
URL: https://github.com/apache/camel/pull/4384
Hi,
We met a condition in production where the Connection Factory of Rabbitmq won't recover (when isAutomaticRecoveryEnabled is set to true). It's easily reproducible by calling the ".close()" method on it.
It's explained here : https://www.rabbitmq.com/api-guide.html#recovery
The current code would always returns the same connection, not re-establishing it, if isAutomaticRecoveryEnabled is enabled.
Our solution, based on the fact we just have "isOpen" to check the state of the channel/connection was to re-establish the connection in the RabbitMQConsumer even if "isAutomaticRecoveryEnabled" is enabled.
What's your thought about it ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] davsclaus commented on a change in pull request #4384: AutomaticRecovery from RabbitMQ Connection Factory doesn't recover from everything
Posted by GitBox <gi...@apache.org>.
davsclaus commented on a change in pull request #4384:
URL: https://github.com/apache/camel/pull/4384#discussion_r501562424
##########
File path: components/camel-rabbitmq/src/main/java/org/apache/camel/component/rabbitmq/RabbitConsumer.java
##########
@@ -326,9 +340,13 @@ public void reconnect() throws Exception {
} else if (channel == null || !isAutomaticRecoveryEnabled()) {
LOG.info("Attempting to open a new rabbitMQ channel");
Connection conn = consumer.getConnection();
- channel = openChannel(conn);
- // Register the channel to the tag
- start();
+ try {
+ stop();
Review comment:
Just wonder what to do if stop throw an exception, should we silently ignore this?
##########
File path: components/camel-rabbitmq/src/main/java/org/apache/camel/component/rabbitmq/RabbitConsumer.java
##########
@@ -326,9 +340,13 @@ public void reconnect() throws Exception {
} else if (channel == null || !isAutomaticRecoveryEnabled()) {
LOG.info("Attempting to open a new rabbitMQ channel");
Connection conn = consumer.getConnection();
- channel = openChannel(conn);
- // Register the channel to the tag
- start();
+ try {
+ stop();
Review comment:
Yeah that sounds okay lets go with this - its better than whats before
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] bedlaj commented on pull request #4384: AutomaticRecovery from RabbitMQ Connection Factory doesn't recover from everything
Posted by GitBox <gi...@apache.org>.
bedlaj commented on pull request #4384:
URL: https://github.com/apache/camel/pull/4384#issuecomment-705095840
Thanks for contribution. Can you please revert all unnecessary changes? You have reordered methods and fields in `RabbitConsumer` and because of that is your PR much harder to review then it could be.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] JSchoreels commented on a change in pull request #4384: AutomaticRecovery from RabbitMQ Connection Factory doesn't recover from everything
Posted by GitBox <gi...@apache.org>.
JSchoreels commented on a change in pull request #4384:
URL: https://github.com/apache/camel/pull/4384#discussion_r501596540
##########
File path: components/camel-rabbitmq/src/main/java/org/apache/camel/component/rabbitmq/RabbitConsumer.java
##########
@@ -326,9 +340,13 @@ public void reconnect() throws Exception {
} else if (channel == null || !isAutomaticRecoveryEnabled()) {
LOG.info("Attempting to open a new rabbitMQ channel");
Connection conn = consumer.getConnection();
- channel = openChannel(conn);
- // Register the channel to the tag
- start();
+ try {
+ stop();
Review comment:
The case I had during my integration test was that in the stop(), there is a call to "basicCancel", which throws an unhandled exception if the channel or the connection was already closed (in my case due to the fact I called this.channel.getConnection().close() with the debugger during the handling of the message), which lead to "start()" never called, and thus the consumers never handled new message afterward.
Maybe the exception handling could be more specific to the basicCancel and not on all the "stop()", but I thought that start should always be called no matter how stop finishes.
I'm open to other way of handling that.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] davsclaus merged pull request #4384: AutomaticRecovery from RabbitMQ Connection Factory doesn't recover from everything
Posted by GitBox <gi...@apache.org>.
davsclaus merged pull request #4384:
URL: https://github.com/apache/camel/pull/4384
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] JSchoreels commented on a change in pull request #4384: AutomaticRecovery from RabbitMQ Connection Factory doesn't recover from everything
Posted by GitBox <gi...@apache.org>.
JSchoreels commented on a change in pull request #4384:
URL: https://github.com/apache/camel/pull/4384#discussion_r501596540
##########
File path: components/camel-rabbitmq/src/main/java/org/apache/camel/component/rabbitmq/RabbitConsumer.java
##########
@@ -326,9 +340,13 @@ public void reconnect() throws Exception {
} else if (channel == null || !isAutomaticRecoveryEnabled()) {
LOG.info("Attempting to open a new rabbitMQ channel");
Connection conn = consumer.getConnection();
- channel = openChannel(conn);
- // Register the channel to the tag
- start();
+ try {
+ stop();
Review comment:
The case I had during my integration test was that in the stop(), there is a call to "basicCancel", which throws an unhandled exception (AlreadyClosedException if I recall correctly) if the channel or the connection was already closed (in my case due to the fact I called this.channel.getConnection().close() with the debugger during the handling of the message), which lead to "start()" never called, and thus the consumers never handled new message afterward.
Maybe the exception handling could be more specific to the basicCancel and not on all the "stop()", but I thought that start should always be called no matter how stop finishes.
I'm open to other way of handling that.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] JSchoreels commented on pull request #4384: AutomaticRecovery from RabbitMQ Connection Factory doesn't recover from everything
Posted by GitBox <gi...@apache.org>.
JSchoreels commented on pull request #4384:
URL: https://github.com/apache/camel/pull/4384#issuecomment-704959009
I added few others improvements we made, specially when the Consumer receives a RPC Call, he can't send it back if the channel is down ... So we added a little mechanism to reconnect the channel one time before retrying to send back the reply. It allowed us to not lose any RPC call in case of connection/channel outage.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] JSchoreels commented on pull request #4384: AutomaticRecovery from RabbitMQ Connection Factory doesn't recover from everything
Posted by GitBox <gi...@apache.org>.
JSchoreels commented on pull request #4384:
URL: https://github.com/apache/camel/pull/4384#issuecomment-705412267
I pushed a new commit which revert all the format changes, "Files changed" looks normal now
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] JSchoreels commented on a change in pull request #4384: AutomaticRecovery from RabbitMQ Connection Factory doesn't recover from everything
Posted by GitBox <gi...@apache.org>.
JSchoreels commented on a change in pull request #4384:
URL: https://github.com/apache/camel/pull/4384#discussion_r501596540
##########
File path: components/camel-rabbitmq/src/main/java/org/apache/camel/component/rabbitmq/RabbitConsumer.java
##########
@@ -326,9 +340,13 @@ public void reconnect() throws Exception {
} else if (channel == null || !isAutomaticRecoveryEnabled()) {
LOG.info("Attempting to open a new rabbitMQ channel");
Connection conn = consumer.getConnection();
- channel = openChannel(conn);
- // Register the channel to the tag
- start();
+ try {
+ stop();
Review comment:
The case I had during my integration test was that in the stop(), there is a call to "basicCancel", which throws an unhandled exception if the channel or the connection was already closed, which lead to "start()" never called, and thus the consumers never handled new message afterward.
Maybe the exception handling could be more specific to the basicCancel and not on all the "stop()", but I thought that start should always be called no matter how stop finishes.
I'm open to other way of handling that.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] davsclaus merged pull request #4384: AutomaticRecovery from RabbitMQ Connection Factory doesn't recover from everything
Posted by GitBox <gi...@apache.org>.
davsclaus merged pull request #4384:
URL: https://github.com/apache/camel/pull/4384
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] JSchoreels commented on a change in pull request #4384: AutomaticRecovery from RabbitMQ Connection Factory doesn't recover from everything
Posted by GitBox <gi...@apache.org>.
JSchoreels commented on a change in pull request #4384:
URL: https://github.com/apache/camel/pull/4384#discussion_r501596540
##########
File path: components/camel-rabbitmq/src/main/java/org/apache/camel/component/rabbitmq/RabbitConsumer.java
##########
@@ -326,9 +340,13 @@ public void reconnect() throws Exception {
} else if (channel == null || !isAutomaticRecoveryEnabled()) {
LOG.info("Attempting to open a new rabbitMQ channel");
Connection conn = consumer.getConnection();
- channel = openChannel(conn);
- // Register the channel to the tag
- start();
+ try {
+ stop();
Review comment:
The case I had during my integration test was that in the stop(), there is a call to "basicCancel", which throws an unhandled exception if the channel or the connection was already closed, which lead to "start()" never called, and thus the consumers never handled new message afterward.
Maybe the exception handling could be more specific to the basicCancel and not on all the "stop()", but I thought that start should always be called no matter how stop finishes.
I'm open to other way of handling that.
##########
File path: components/camel-rabbitmq/src/main/java/org/apache/camel/component/rabbitmq/RabbitConsumer.java
##########
@@ -326,9 +340,13 @@ public void reconnect() throws Exception {
} else if (channel == null || !isAutomaticRecoveryEnabled()) {
LOG.info("Attempting to open a new rabbitMQ channel");
Connection conn = consumer.getConnection();
- channel = openChannel(conn);
- // Register the channel to the tag
- start();
+ try {
+ stop();
Review comment:
The case I had during my integration test was that in the stop(), there is a call to "basicCancel", which throws an unhandled exception if the channel or the connection was already closed (in my case due to the fact I called this.channel.getConnection().close() with the debugger during the handling of the message), which lead to "start()" never called, and thus the consumers never handled new message afterward.
Maybe the exception handling could be more specific to the basicCancel and not on all the "stop()", but I thought that start should always be called no matter how stop finishes.
I'm open to other way of handling that.
##########
File path: components/camel-rabbitmq/src/main/java/org/apache/camel/component/rabbitmq/RabbitConsumer.java
##########
@@ -326,9 +340,13 @@ public void reconnect() throws Exception {
} else if (channel == null || !isAutomaticRecoveryEnabled()) {
LOG.info("Attempting to open a new rabbitMQ channel");
Connection conn = consumer.getConnection();
- channel = openChannel(conn);
- // Register the channel to the tag
- start();
+ try {
+ stop();
Review comment:
The case I had during my integration test was that in the stop(), there is a call to "basicCancel", which throws an unhandled exception (AlreadyClosedException if I recall correctly) if the channel or the connection was already closed (in my case due to the fact I called this.channel.getConnection().close() with the debugger during the handling of the message), which lead to "start()" never called, and thus the consumers never handled new message afterward.
Maybe the exception handling could be more specific to the basicCancel and not on all the "stop()", but I thought that start should always be called no matter how stop finishes.
I'm open to other way of handling that.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] JSchoreels commented on pull request #4384: AutomaticRecovery from RabbitMQ Connection Factory doesn't recover from everything
Posted by GitBox <gi...@apache.org>.
JSchoreels commented on pull request #4384:
URL: https://github.com/apache/camel/pull/4384#issuecomment-705104507
Damn, I overlooked that. I fix it tomorrow, sorry.
Le mer. 7 oct. 2020 à 19:50, Jan Bednar <no...@github.com> a écrit :
> Thanks for contribution. Can you please revert all unnecessary changes?
> You have reordered methods and fields in RabbitConsumer and because of
> that is your PR much harder to review then it could be.
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/camel/pull/4384#issuecomment-705095840>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ACLMI2CV4QUZ4W23L4R3WALSJSS5ZANCNFSM4SHMSMHA>
> .
>
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] davsclaus commented on a change in pull request #4384: AutomaticRecovery from RabbitMQ Connection Factory doesn't recover from everything
Posted by GitBox <gi...@apache.org>.
davsclaus commented on a change in pull request #4384:
URL: https://github.com/apache/camel/pull/4384#discussion_r501864607
##########
File path: components/camel-rabbitmq/src/main/java/org/apache/camel/component/rabbitmq/RabbitConsumer.java
##########
@@ -326,9 +340,13 @@ public void reconnect() throws Exception {
} else if (channel == null || !isAutomaticRecoveryEnabled()) {
LOG.info("Attempting to open a new rabbitMQ channel");
Connection conn = consumer.getConnection();
- channel = openChannel(conn);
- // Register the channel to the tag
- start();
+ try {
+ stop();
Review comment:
Yeah that sounds okay lets go with this - its better than whats before
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] davsclaus commented on a change in pull request #4384: AutomaticRecovery from RabbitMQ Connection Factory doesn't recover from everything
Posted by GitBox <gi...@apache.org>.
davsclaus commented on a change in pull request #4384:
URL: https://github.com/apache/camel/pull/4384#discussion_r501562424
##########
File path: components/camel-rabbitmq/src/main/java/org/apache/camel/component/rabbitmq/RabbitConsumer.java
##########
@@ -326,9 +340,13 @@ public void reconnect() throws Exception {
} else if (channel == null || !isAutomaticRecoveryEnabled()) {
LOG.info("Attempting to open a new rabbitMQ channel");
Connection conn = consumer.getConnection();
- channel = openChannel(conn);
- // Register the channel to the tag
- start();
+ try {
+ stop();
Review comment:
Just wonder what to do if stop throw an exception, should we silently ignore this?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] JSchoreels commented on pull request #4384: AutomaticRecovery from RabbitMQ Connection Factory doesn't recover from everything
Posted by GitBox <gi...@apache.org>.
JSchoreels commented on pull request #4384:
URL: https://github.com/apache/camel/pull/4384#issuecomment-705412267
I pushed a new commit which revert all the format changes, "Files changed" looks normal now
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org