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
Retry disconnects and expired sessions when reading system.zookeeper #59388
Conversation
This is an automated comment for commit 6e82dc8 with description of existing statuses. It's updated for the latest CI running ❌ Click here to open a full report in a separate page Successful checks
|
template <typename Result, typename Operation> | ||
static Result runWithReconnects(Operation && operation, ContextPtr context, QueryStatusPtr query_status) | ||
{ | ||
constexpr int max_retries = 20; /// Limit retries by some reasonable number to avoid infinite loops |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why isn't this using ZooKeeperRetriesControl
? It's better to adapt that if we need to rather than re-implement retries
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
Do you think we should move ZooKeeperRetriesControl from Storages/MergeTree to Common?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely. Ideally we should be using ZooKeeperRetriesControl
+ ZooKeeperWithFaultInjectionPtr
everywhere so we handle both injections and retries.
@@ -426,7 +427,7 @@ void ReadFromSystemZooKeeper::applyFilters() | |||
|
|||
void ReadFromSystemZooKeeper::fillData(MutableColumns & res_columns) | |||
{ | |||
zkutil::ZooKeeperPtr zookeeper = context->getZooKeeper(); | |||
QueryStatusPtr query_status = context->getProcessListElement(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use the whole pack:
- Use
insert_keeper_max_retries
and similar for settings. I hope we will unify it withbackup_restore_keeper_max_retries
and have a single common setting for retries at some point, so we shouldn't add a new one. - Use
ZooKeeperWithFaultInjection
and refresh it in the loop if necessary.
7b1d576
to
e01cca9
Compare
e01cca9
to
aba749d
Compare
aba749d
to
7c1e318
Compare
73715b2
to
6e82dc8
Compare
Integration tests (tsan) #59415 |
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Retry disconnects and expired sessions when reading
system.zookeeper
. This is helpful when reading many rows fromsystem.zookeeper
table especially in the presence of fault-injected disconnects.