[feat] [broker] PIP-188: Support option to disconnect clients that not support cluster migration feature #20084
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…r migration feature
rdhabalia
changed the title
|
The pr had no activity for 30 days, mark with Stale label. |
|
@eolivelli This PR is needed for client proto version enforcement |
| + "might be required by broker to apply globally on all topics." | ||
| + "(eg: all clients must be on V20 to perform cloud migration)" | ||
| ) | ||
| private int clientMinVersionAllowed = -1; |
There was a problem hiding this comment.
Would it make sense to add proto or protocol to the name to disambiguate it from the library version?
| doc = "Minimum client version allowed by broker else broker will reject connection." | ||
| + "(It's useful when client lib doesn't support specific feature and feature " | ||
| + "might be required by broker to apply globally on all topics." | ||
| + "(eg: all clients must be on V20 to perform cloud migration)" |
There was a problem hiding this comment.
Since it's an int, should we just say 20, not V20?
eolivelli
approved these changes
Sep 19, 2023
| log.info("[{}] client with version {} must be upgraded to {}", remoteAddress, clientProtoVersion, | ||
| clientMinVersionAllowed); | ||
| final ByteBuf msg = Commands.newError(-1, ServerError.UnsupportedVersionError, | ||
| "Upgrade version to " + clientMinVersionAllowed + " or higher"); |
There was a problem hiding this comment.
What about 'upgrade your client to a version that supports protocol version ...'
Otherwise it seems that you have to upgrade to Pulsar 20
|
The pr had no activity for 30 days, mark with Stale label. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
As discussed at PIP-188 and in PR #19605
In ordered to achieve cluster migration, all clients need to be upgraded to the version which handles cluster migration redirection. So, Broker can't complete migration if clients are not upgrade to the supported version. Therefore, it's really important to find out if any unsupported client is connected or don't allow any unsupported client to connect if we want to start the cluster migration process. Therefore, we will add a flag to allow the broker to reject all client connections which are initiated from unsupported clients. The broker will not allow connection from clients which don't support cluster migration redirection handling if the
clientMinVersionAllowedbroker flag is enabled.Modifications
Add flag
clientMinVersionAllowedto allow brokers to ocreate connections with only those clients that support cluster migration handling feature to avoid incomplete cluster migration process.Verifying this change
(Please pick either of the following options)
This change is a trivial rework / code cleanup without any test coverage.
(or)
This change is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
(example:)
Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
Documentation
docdoc-requireddoc-not-neededdoc-completeMatching PR in forked repository
PR in forked repository: