Add MultiRead support in Keeper and internal ZK client#41410
Add MultiRead support in Keeper and internal ZK client#41410
Conversation
|
The MultiRead supported by ZooKeeper supports only 2 operations:
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. |
| }; | ||
|
|
||
| template <bool with_multiread> | ||
| std::vector<BlockInfoInZooKeeper> getBlockInfos(const auto & partitions, const auto & zookeeper, const auto & zookeeper_path) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| if (operation_type.has_value() && *operation_type != type) | ||
| throw Exception("Illegal mixing of read and write operations in multi request", Error::ZBADARGUMENTS); |
There was a problem hiding this comment.
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
|
@tavplubix I would like to hear your thoughts on something. |
|
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 |
|
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'. |
|
I added the discussed behavior of always processing every read request, and not stopping on the first failure. |
Changelog category (leave one):
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