Preserve current endpoint across failed reconnects - #558
Merged
Conversation
slabko
force-pushed
the
invalidate-connection-on-exception-2
branch
from
August 28, 2026 16:16
cda1ce4 to
1918252
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR clarifies and stabilizes “current endpoint” semantics by making it represent the last successfully connected endpoint, and updates reconnect/failover logic and tests to match that definition.
Changes:
- Preserve the last successfully connected endpoint across failed reconnect/failover attempts.
- Make reconnect ordering consistent (retry current first, then fail over) and validate non-empty endpoint lists in the round-robin iterator.
- Add unit test coverage for reconnect and failover ordering via a socket-factory adapter.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
ut/test_socket_factory_adapters.h |
Adds a socket factory adapter to record attempted endpoints and inject one-time failures for testing. |
ut/client_ut.cpp |
Replaces/extends reconnect tests to assert “retry current before failover” ordering and wraparound behavior. |
ut/BUILD.bazel |
Includes the new test helper header in the Bazel test target. |
clickhouse/client.h |
Updates API documentation to reflect “current endpoint = last successful endpoint” semantics. |
clickhouse/client.cpp |
Implements new endpoint preservation and ordering in reconnect/failover paths, and adjusts retry behavior. |
clickhouse/base/endpoints_iterator.cpp |
Adds validation to reject empty endpoint lists at iterator construction. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
616
to
+620
| } catch (const std::system_error&) { | ||
| if (++i == options_.endpoints.size()) | ||
| current_endpoint_ = endpoints_iterator->Next(); | ||
| if (i >= options_.endpoints.size()) | ||
| { | ||
| current_endpoint_.reset(); | ||
| current_endpoint_ = last_endpoint; |
Contributor
Author
There was a problem hiding this comment.
That preserves the existing behavior
| try { | ||
| socket_factory_->sleepFor(options_.retry_timeout); | ||
| ResetConnection(); | ||
| } catch (const std::system_error&) { |
Contributor
Author
There was a problem hiding this comment.
This is consistent with other Reconnect* functions
slabko
force-pushed
the
invalidate-connection-on-exception-2
branch
from
August 28, 2026 17:25
7e1a947 to
1918252
Compare
Keep the current endpoint set to the last successfully connected endpoint. ResetConnection retries only that endpoint, while ResetConnectionEndpoint tries it first before failing over to the remaining endpoints. Restore the previous endpoint if all reconnection attempts fail. Validate non-empty endpoint lists in RoundRobinEndpointsIterator and add coverage for reconnect and failover ordering.
slabko
force-pushed
the
invalidate-connection-on-exception-2
branch
from
August 28, 2026 17:26
1918252 to
4e6c88c
Compare
joe-clickhouse
approved these changes
Aug 28, 2026
This file contains hidden or 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
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.
Background
The meaning of the optional current endpoint was confusing. Previously, it became empty only after a failed reconnection attempt, so its value did not consistently describe the client state or make it clear which endpoint would be tried next.
The current endpoint now represents the last successfully connected endpoint and always contains a value for a constructed client. It remains optional only for backward compatibility. This also defines reconnection behavior explicitly:
ResetConnectionretries only the current endpoint and does not perform failover.ResetConnectionEndpointtries the current endpoint first, then the other configured endpoints.Summary
RoundRobinEndpointsIterator