Skip to content

Add MultiRead support in Keeper and internal ZK client#41410

Merged
tavplubix merged 19 commits intomasterfrom
keeper-multiread
Oct 3, 2022
Merged

Add MultiRead support in Keeper and internal ZK client#41410
tavplubix merged 19 commits intomasterfrom
keeper-multiread

Conversation

@antonio2368
Copy link
Copy Markdown
Member

@antonio2368 antonio2368 commented Sep 16, 2022

Changelog category (leave one):

  • Improvement

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Add support for MultiRead in Keeper and internal ZooKeeper client.

Continuation of #36725

Information about CI checks: https://clickhouse.com/docs/en/development/continuous-integration/

antonio2368
antonio2368 commented Sep 16, 2022
View reviewed changes
@antonio2368
Copy link
Copy Markdown
Member Author

antonio2368 commented Sep 16, 2022

The MultiRead supported by ZooKeeper supports only 2 operations:

  • Get
  • Old version of list which we call SimpleList
    This is the only place where the old version of the list (without stat) is used.
    I don't see why we wouldn't be able to send multiple regular list operations in MultiRead and other read requests like exists.

Because of that and because it's hard for us to differentiate when are we connected to ZK or Keeper, I propose we support MultiRead operations only when we connect to Keeper which supports it.

cc @tavplubix @alesapin

@antonio2368 antonio2368 mentioned this pull request Sep 16, 2022
Closed
4 tasks
@antonio2368 antonio2368 changed the title Keeper multiread Add MultiRead support in Keeper and internal ZK client Sep 16, 2022
@robot-clickhouse robot-clickhouse added the pr-improvement Pull request with some product improvements label Sep 16, 2022
@antonio2368 antonio2368 marked this pull request as ready for review September 27, 2022 12:39
@tavplubix tavplubix self-assigned this Sep 27, 2022
tavplubix
tavplubix approved these changes Sep 28, 2022
View reviewed changes
src/Storages/MergeTree/ReplicatedMergeTreeQueue.cpp Outdated
};

template <bool with_multiread>
std::vector<BlockInfoInZooKeeper> getBlockInfos(const auto & partitions, const auto & zookeeper, const auto & zookeeper_path)
Copy link
Copy Markdown
Member

@tavplubix tavplubix Sep 28, 2022

Choose a reason for hiding this comment

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

There are much more places where we send multiple get requests asynchronously. Consider implementing such method in ZooKeeper client, that will take parent path and array (or iterators range) of node names, check API version and retrieve data using async get requests or MultiRead

Copy link
Copy Markdown
Member Author

@antonio2368 antonio2368 Sep 28, 2022

Choose a reason for hiding this comment

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

I've been thinking about it, but didn't do it as part of this task because it could end up a bit complex.
Mostly because of the way how we process with async gets, when we get the result of the next get request we instantly process it, we don't wait for all of them to finish.

One way to implement it would be to send a callback to be called on each get request but I can do it as part of a different PR.

src/Common/ZooKeeper/ZooKeeperCommon.cpp Outdated
Comment on lines +440 to +441
if (operation_type.has_value() && *operation_type != type)
throw Exception("Illegal mixing of read and write operations in multi request", Error::ZBADARGUMENTS);
Copy link
Copy Markdown
Member

@tavplubix tavplubix Sep 28, 2022

Choose a reason for hiding this comment

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

It means that our code tried to send an ill-formed request. Consider throwing LOGICAL_ERROR (but it will be DB::Exception, not Coordination::Exception then) or adding chassert here

@antonio2368
Copy link
Copy Markdown
Member Author

antonio2368 commented Sep 28, 2022

@tavplubix I would like to hear your thoughts on something.
When I checked async read calls, some of them are okay with ZNONODE.
Now, MultiRead behaves same as Multi, if there is any request that is not ZOK, rest of request won't be processed and return ZRUNTIME. It makes sense for write requests because we can get invalid data if we don't abort on a faulty request but for read requests it doesn't matter.
Keeping it like that is more consistent but as we support MultiRead on Keeper only, we can change that behavior process all requests no matter what is the returned value.

@tavplubix
Copy link
Copy Markdown
Member

tavplubix commented Sep 28, 2022

I think it would be more convenient to return responses for all read requests even if some nodes do not exist. Sometimes we need to get many nodes which may be being removed concurrently (and it's not an error if some node does not exist). But multi should throw ZNONODE exception in this case anyway (and tryMulti should fill all responses with either data or error code)

@antonio2368
Copy link
Copy Markdown
Member Author

antonio2368 commented Sep 28, 2022 •
edited
Loading

So on the Keeper's side, it should set the error code of the first detected response which is not ZOK but continue processing and the client will handle it differently depending on if it's `multi/tryMulti'.
This should allow us to cover much more cases, and add support for exists for example.

@antonio2368 antonio2368 marked this pull request as draft September 29, 2022 06:12
@antonio2368 antonio2368 marked this pull request as ready for review September 29, 2022 11:27
@antonio2368
Copy link
Copy Markdown
Member Author

antonio2368 commented Sep 29, 2022 •
edited
Loading

I added the discussed behavior of always processing every read request, and not stopping on the first failure.
Also, I created API for running multiple read requests of the same type on some paths (it picks which method to use based on the server support).
It may look complicated, but I tried to hide knowledge of which method was used as much as possible, and to me, it looks easy to use.
Get, exists and getChildren are all supported.
The 3 changes I made are more for showcasing the API. I'll continue in different PR searching for places where I can use it instead of async read requests.

@tavplubix
Copy link
Copy Markdown
Member

tavplubix commented Oct 3, 2022

Integration tests - test_disks_app_func was broken in master
Stateless tests (aarch64) - #41934
Stress test (msan) - #41548
Stress test (tsan) - The specified key does not exist in BC check

@tavplubix tavplubix merged commit 00914a1 into master Oct 3, 2022
@tavplubix tavplubix deleted the keeper-multiread branch October 3, 2022 11:17
@alexey-milovidov alexey-milovidov mentioned this pull request Oct 8, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tavplubix tavplubix tavplubix approved these changes

Assignees

@tavplubix tavplubix

Labels

pr-improvement Pull request with some product improvements

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@antonio2368 @tavplubix @robot-clickhouse