Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add RetryableConnection for the case for wait a moment when The channelMaxlimit is reached #2556

Merged

Conversation

javaecrainbow
Copy link
Contributor

When I encounter message sending in high-concurrency scenarios in the actual project, I often encounter 'The channelMax limit is reached. Try later.' However, I need to handle this kind of retry compatibility at different parts of the business logic. So, I'm considering extending support for retry connection types in Spring RabbitMQ

@pivotal-cla
Copy link

@javaecrainbow Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-cla
Copy link

@javaecrainbow Thank you for signing the Contributor License Agreement!

Copy link
Contributor

@garyrussell garyrussell left a comment

Choose a reason for hiding this comment

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

Please add an entry to whats-new.adoc and update the connection section in amqp.adoc.

@garyrussell
Copy link
Contributor

garyrussell commented Nov 20, 2023

I have found the corresponding solution in the relevant amqp.adoc document, so I am closing this pull request.

I assume you mean by adding a suitably configured RetryTemplate to the RabbitTemplate https://docs.spring.io/spring-amqp/docs/current/reference/html/#template-retry

So why did you re-open it?

@javaecrainbow
Copy link
Contributor Author

Indeed, there are scenarios where the RetryTemplate in RabbitTemplate can be used to handle custom exception retries at a higher level. However, there are some use cases not triggered from RabbitTemplate. For instance, in BlockingQueueConsumer, there is also a business scenario for obtaining a channel, but it does not support RetryTemplate. Therefore, it is necessary to support RetryTemplate uniformly in the lower-level Connection module to cater to various upper-level business use cases that involve retrying channel acquisition exceptions. @garyrussell

@artembilan
Copy link
Member

The BlockingQueueConsumer (essentially any AbstractMessageListenerContainer) can be supplied with a recoveryBackOff:

	/**
	 * Specify the {@link BackOff} for interval between recovery attempts.
	 * The default is 5000 ms, that is, 5 seconds.
	 * With the {@link BackOff} you can supply the {@code maxAttempts} for recovery before
	 * the {@link #stop()} will be performed.
	 * @param recoveryBackOff The BackOff to recover.
	 * @since 1.5
	 */
	public void setRecoveryBackOff(BackOff recoveryBackOff) {

Which works as a retry on connection failure as well.

So, looks like we have both passive and active connection covered with retries.

Any thought why do we need this extra one exactly in the connection?

Either way, if we really need, I'd prefer to make it based on that BackOff abstraction instead.
The RetryTemplate is more high-level, end-user API. What we are talking about here is really a rudimentary back off between connection attempts.

Let us know if that is OK with you and we close this as Works as Designed!

@javaecrainbow
Copy link
Contributor Author

The BlockingQueueConsumer (essentially any AbstractMessageListenerContainer) can be supplied with a recoveryBackOff:

	/**
	 * Specify the {@link BackOff} for interval between recovery attempts.
	 * The default is 5000 ms, that is, 5 seconds.
	 * With the {@link BackOff} you can supply the {@code maxAttempts} for recovery before
	 * the {@link #stop()} will be performed.
	 * @param recoveryBackOff The BackOff to recover.
	 * @since 1.5
	 */
	public void setRecoveryBackOff(BackOff recoveryBackOff) {

Which works as a retry on connection failure as well.

So, looks like we have both passive and active connection covered with retries.

Any thought why do we need this extra one exactly in the connection?

Either way, if we really need, I'd prefer to make it based on that BackOff abstraction instead. The RetryTemplate is more high-level, end-user API. What we are talking about here is really a rudimentary back off between connection attempts.

Let us know if that is OK with you and we close this as Works as Designed!

I encountered the problem in the case of the connection is shared between producers and consumers, and enabled producer reliability delivery . In the case of large concurrent message delivery, the connection cannot be obtained intermittently from RabbitTemplate . From the code you referred, I think that the solution you provide is actually a retry mechanism for consuming modules. I don't know whether there's a problem with my understanding, or whether spring-rabbit already has a solution for my problem in message delivery

@artembilan
Copy link
Member

But you said it yourself before:

there are scenarios where the RetryTemplate in RabbitTemplate can be used to handle custom exception retries at a higher level.

So, for RabbitTemplate you use RetryTemplate since it is passive operation and called from other code.
For the consumer side we use that BackOff functionality.

Not sure what else you'd like to have on top of existing functionality without over-confusing framework users with paradox of choice.

@javaecrainbow
Copy link
Contributor Author

javaecrainbow commented Feb 6, 2024

But you said it yourself before:

there are scenarios where the RetryTemplate in RabbitTemplate can be used to handle custom exception retries at a higher level.

So, for RabbitTemplate you use RetryTemplate since it is passive operation and called from other code. For the consumer side we use that BackOff functionality.

Not sure what else you'd like to have on top of existing functionality without over-confusing framework users with paradox of choice.

Thanks, I understand you way you provided can solve my problem I encountered before. Just I think the exception handling function at the network level should belong to the low-level design, so it should also has its own exception handling strategy mechanism at the bottom layer, and RetryTemplate and BackOff are modules at the upper level.

@artembilan
Copy link
Member

OK. I see your point:

			Channel channel0 = super.getDelegate().createChannel();
				if (channel0 == null) {
					throw new AmqpResourceNotAvailableException("The channelMax limit is reached. Try later.");
				}

Can we then still look into that BackOff injection instead of heavy RetryTemplate?
As you said: this is low-level, so no need in sophisticated retry features.

Thanks

@javaecrainbow
Copy link
Contributor Author

OK. I see your point:

			Channel channel0 = super.getDelegate().createChannel();
				if (channel0 == null) {
					throw new AmqpResourceNotAvailableException("The channelMax limit is reached. Try later.");
				}

Can we then still look into that BackOff injection instead of heavy RetryTemplate? As you said: this is low-level, so no need in sophisticated retry features.

Thanks

ok,i agree that. Wish backoff injection function in connection module. whether i close the pr ?

@artembilan
Copy link
Member

I think you can just make a change and push the next commit.
Thanks

…WithRetry

# Conflicts:
#	src/reference/antora/modules/ROOT/pages/whats-new.adoc
#	src/reference/asciidoc/amqp.adoc
@javaecrainbow
Copy link
Contributor Author

@artembilan i have changed the retry template to backoff util and commited

Copy link
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

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

Thank you for the progress in PR!

There is a number of Checkstyle violations:

Error: eckstyle] [ERROR] /home/runner/work/spring-amqp/spring-amqp/spring-rabbit/src/main/java/org/springframework/amqp/rabbit/connection/AbstractConnectionFactory.java:66:1: Wrong order for 'org.springframework.util.backoff.BackOff' import. [ImportOrder]
Error: eckstyle] [ERROR] /home/runner/work/spring-amqp/spring-amqp/spring-rabbit/src/main/java/org/springframework/amqp/rabbit/connection/AbstractConnectionFactory.java:168: Line matches the illegal pattern 'Trailing whitespace'. [Regexp]
Error: eckstyle] [ERROR] /home/runner/work/spring-amqp/spring-amqp/spring-rabbit/src/main/java/org/springframework/amqp/rabbit/connection/AbstractConnectionFactory.java:591:41: Reference to instance variable 'connectionCreatingBackOff' needs "this.". [RequireThis]
Error: eckstyle] [ERROR] /home/runner/work/spring-amqp/spring-amqp/spring-rabbit/src/main/java/org/springframework/amqp/rabbit/connection/AbstractConnectionFactory.java:591:84: Reference to instance variable 'connectionCreatingBackOff' needs "this.". [RequireThis]
Error: eckstyle] [ERROR] /home/runner/work/spring-amqp/spring-amqp/spring-rabbit/src/main/java/org/springframework/amqp/rabbit/connection/SimpleConnection.java:4: Line does not match expected header line of '^\Q * Licensed under the Apache License, Version 2.0 (the "License");\E$'. [RegexpHeader]
Error: eckstyle] [ERROR] /home/runner/work/spring-amqp/spring-amqp/spring-rabbit/src/main/java/org/springframework/amqp/rabbit/connection/SimpleConnection.java:28:1: Wrong order for 'org.springframework.util.backoff.BackOffExecution' import. [ImportOrder]
Error: eckstyle] [ERROR] /home/runner/work/spring-amqp/spring-amqp/spring-rabbit/src/main/java/org/springframework/amqp/rabbit/connection/SimpleConnection.java:65:51: Reference to instance variable 'backOffExecution' needs "this.". [RequireThis]
Error: eckstyle] [ERROR] /home/runner/work/spring-amqp/spring-amqp/spring-rabbit/src/main/java/org/springframework/amqp/rabbit/connection/SimpleConnection.java:66:49: Reference to instance variable 'backOffExecution' needs "this.". [RequireThis]
Error: eckstyle] [ERROR] /home/runner/work/spring-amqp/spring-amqp/spring-rabbit/src/main/java/org/springframework/amqp/rabbit/connection/SimpleConnection.java:108: Line matches the illegal pattern 'Trailing whitespace'. [Regexp]

Please, use gradlew check command locally before pushing commits to PR.

Copy link
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

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

Please, add your name to the @author list of all the affected classes.

Copy link
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

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

Just a couple minor comments.
Fixing on merge...

Thanks

@artembilan artembilan merged commit d135246 into spring-projects:main Mar 13, 2024
3 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.

6 participants