-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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 ability to skip read-only replicas for INSERT into Distributed engine #59176
Conversation
This is an automated comment for commit 4eafb86 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
|
05d599a
to
dba12a9
Compare
Test failures does not looks related:
|
c47efd1
to
6d6129c
Compare
|
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.
I think, the new behavior requires a setting. In general, writing into a currently read-only replication table is covered by insert retries.
The question is what semantic we want to bring with the new setting
(1) - prefer insert into currently non-read-only replicas
or
(2) - use only non-read-only replicas
AFAIU, the current implementation is about (1)
src/Storages/Distributed/DistributedAsyncInsertDirectoryQueue.cpp
Outdated
Show resolved
Hide resolved
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.
I think, the new behavior requires a setting. In general, writing into a currently read-only replication table is covered by insert retries.
The question is what semantic we want to bring with the new setting
(1) - prefer insert into currently non-read-only replicas
or
(2) - use only non-read-only replicas
AFAIU, the current implementation is about (1)
My thinking was that since it only prefers non-read-only replicas, there is no need in a separate setting, but I'm OK with adding one.
And yes, if there will be a setting then we can add a enum for it, i.e.
Yes. |
Let's keep it. Doubt that we need (2) actually |
Do you still think that extra setting make sense? |
Hard to say, but at least there will be a way to control behavior. I guess there could be cases which benefit or not from this behavior (I guess it's mostly about load distribution). For example, if several long-running insert queries are thrown while some replicas will reconnect to zk, the inserts will be assigned to non-read-only replicas, i.e. load could not be distributed well. When the user definitely knows that some replicas will be read-only for a long time, then it is worth excluding read-only replicas upfront. But then, we're talking about option (2) 🤔 |
6d6129c
to
bc48d3e
Compare
Okay, since you are also struggling about new setting for this functionality, I've added enum, And also addressed all of your comments. |
307d8e7
to
ca1c845
Compare
Test failures does not looks related:
|
ca1c845
to
cc155ad
Compare
89f536b
to
cdb2d36
Compare
tests/queries/0_stateless/02980_dist_insert_readonly_replica.sql.j2
Outdated
Show resolved
Hide resolved
tests/queries/0_stateless/02980_dist_insert_readonly_replica.sql.j2
Outdated
Show resolved
Hide resolved
tests/queries/0_stateless/02980_dist_insert_readonly_replica.sql.j2
Outdated
Show resolved
Hide resolved
014a019
to
52f7dd3
Compare
…rite-once) Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
Sometimes replica may be readonly for a long time, and this will make some INSERT queries fail, but it does not make sense to INSERT into readonly replica, so let's ignore them. But note, that this will require to extend TableStatus (not extend, but introduce new version), that will have is_readonly field. Also before background INSERT into Distributed does not uses getManyChecked() which means that they do not request TableStatus packet, while now they would, though this is minor (just a note). v2: Add a note about max_replica_delay_for_distributed_queries for INSERT v3: Skip read-only replicas for async INSERT into Distributed v4: Remove extra @insert parameter for ConnectionPool*::get* It make sense only when the table name had passed -- ConnectionPoolWithFailover::getManyChecked() v5: rebase on top LoggerPtr v6: rebase v7: rebase v8: move TryResult::is_readonly into the end Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
Now, since there is connection probing, the error changed from UNKNOWN_TABLE to ALL_CONNECTION_TRIES_FAILED, though I'm not sure that this good... Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
…ributed v2: replace without s3_plain disk v3: add check for DistributedConnectionReadOnlyReplica v4: set insert_keeper_max_retries=0 v5: do not set insert_keeper_max_retries=0 Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
That was the changes that looks OK for the initial patchset version, but not now, after all review comments. Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
…rt_skip_read_only_replicas v2: fix test Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
CI: https://s3.amazonaws.com/clickhouse-test-reports/59176/fa0c47a42f4187e8284e19d1cd008a0a76c38caf/fast_test.html Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
…und_robin LB) Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
…plica Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
52f7dd3
to
4eafb86
Compare
@@ -289,7 +290,8 @@ void DistributedAsyncInsertBatch::sendSeparateFiles(const SettingsChanges & sett | |||
parent.storage.getContext()->getOpenTelemetrySpanLog()); | |||
|
|||
auto timeouts = ConnectionTimeouts::getTCPTimeoutsWithFailover(insert_settings); | |||
auto connection = parent.pool->get(timeouts); | |||
auto result = parent.pool->getManyCheckedForInsert(timeouts, insert_settings, PoolMode::GET_ONE, parent.storage.remote_storage.getQualifiedName()); |
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.
It's a bit strange to introduce getManyCheckedForInsert
(get many) which only used with PoolMode::GET_ONE
(get one). But it looks fine since requires minimal changes in implementation.
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Add ability to skip read-only replicas for INSERT into Distributed engine (Controlled with
distributed_insert_skip_read_only_replicas
setting, by default OFF - backward compatible)Sometimes replica may be readonly for a long time, and this will make
some INSERT queries fail, but it does not make sense to INSERT into
readonly replica, so let's ignore them.
But note, that this will require to extend TableStatus (not extend, but
introduce new version), that will have is_readonly field.
Also before background INSERT into Distributed does not uses
getManyChecked() which means that they do not request TableStatus
packet, while now they would, though this is minor (just a note).