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

Remove server version validation #1273

Merged
merged 4 commits into from
May 27, 2024
Merged

Conversation

Jarema
Copy link
Member

@Jarema Jarema commented May 27, 2024

If client was created with retry_on_initial_connect and create_consumer_on_stream was called immediately afterwards, it could cause a panic, as semver regex capture would fail on empty string.

This PR removes the validation, as it was never a reliable way to check server version, as the meta leader can be a different version than the server that the client is connected to.
It also protects the server check method against empty semver string.

fixes #1271

Signed-off-by: Tomasz Pietrek tomasz@nats.io

Jarema added 4 commits May 27, 2024 14:25
Signed-off-by: Tomasz Pietrek <tomasz@nats.io>
Checking server version is not a reliable way to know if given
API call is supported for given context, as client might be connected
to a leaf node, while the meta leader is a different version, part of
different cluster.

Signed-off-by: Tomasz Pietrek <tomasz@nats.io>
When retry on initial connect was used, some JetStream consumer calls
could panic in specific conditions because server version was not
available. This test makes sure it will no longer happen.

Signed-off-by: Tomasz Pietrek <tomasz@nats.io>
Signed-off-by: Tomasz Pietrek <tomasz@nats.io>
@Jarema Jarema force-pushed the remove-server-version-validation branch from 6bc6636 to ddb2184 Compare May 27, 2024 13:12
@Jarema Jarema requested a review from caspervonb May 27, 2024 13:22
Copy link
Collaborator

@caspervonb caspervonb left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@piotrpio piotrpio left a comment

Choose a reason for hiding this comment

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

LGTM!

@Jarema Jarema merged commit f86dce9 into main May 27, 2024
12 checks passed
@Jarema Jarema deleted the remove-server-version-validation branch May 27, 2024 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Panic in client.rs, unwrapping None server version.
3 participants