Rendezvous hashing filesystem cache

[ianton-ru]

Improvement object storage cache locality (#708) with rendezvous hashing.

https://en.wikipedia.org/wiki/Rendezvous_hashing

Main change in StorageObjectStorageStableTaskDistributor::getReplicaForFile method.
With original code distribution is not stable when host from beginning of cluster node list is gone or when cluster changes node order by some reason.
With rendezvous hashing best node based on node address (host:port) instead of node number. But a little bit heavy - need do calculate hash N times, one for each node.

[altinity-robot]

This is an automated comment for commit 6092c08 with description of existing statuses. It's updated for the latest CI running

❌ Click here to open a full report in a separate page

Check name	Description	Status
Builds	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	⏳ pending
Integration tests	The integration tests report. In parenthesis the package type is given, and in square brackets are the optional part/total tests	❌ failure
Stateless tests	Runs stateless functional tests for ClickHouse binaries built in various configurations -- release, debug, with sanitizers, etc	❌ failure
Stress test	Runs stateless functional tests concurrently from several clients to detect concurrency-related errors	❌ failure

Successful checks

Check name	Description	Status
Stateful tests	Runs stateful functional tests for ClickHouse binaries built in various configurations -- release, debug, with sanitizers, etc	✅ success

[arthurpassos]

The Rendezvous part is mostly ok, just some small questions.

I am just a bit concerned because ClickHouse#77326 hasn't been merged yet and I believe some commits are missing. Do you plan to cherry-pick the most recent ones as well?

[arthurpassos]

per_replica_pools.empty()

[arthurpassos]

In which case would it not have a value? And why is empty != nullopt?

[ianton-ru]

It must always have value, in ReadFromCluster::createExtension it is called with ids_of_nodes always.

About nullopt - it is the same:

if (var)
if (var.has_value())
if (var != std::nullopt)

First and second are synonyms - https://en.cppreference.com/w/cpp/utility/optional/operator_bool
Third - because comparation with nullopt calls bool operator:

/usr/include/c++/11$ grep -B 5 -A 5 'operator==' optional
...
  // Comparisons with nullopt.
  template<typename _Tp>
    constexpr bool
    operator==(const optional<_Tp>& __lhs, nullopt_t) noexcept
    { return !__lhs; }
...
  template<typename _Tp>
    constexpr bool
    operator==(nullopt_t, const optional<_Tp>& __rhs) noexcept
    { return !__rhs; }

[arthurpassos]

Sorry, I didn't explain my question correctly. I was asking why not just have a vector and check if it is empty instead of an optional of vector

[arthurpassos]

What I mean is that instead of doing:

std::optional<std::vector<std::string>> ids_of_nodes;
...
    if (!ids_of_nodes.has_value())

you simply do:

std::vector<std::string> ids_of_nodes;
...
    if (ids_of_nodes.empty())

I mean, it is ok to keep it as optional, just thought it could be simpler

[arthurpassos]

Nitpick: extract ids_of_nodes into a non-optonal to avoid de-referencing all the time

[ianton-ru]

This is optional in IStorageCluster because all except StorageObjectStorageCluster do not use it.
To make it non-optional need to make a copy of list inside class. Or use smart pointer, than need to check if pointer has value.
Remove some dereferencings.

[arthurpassos]

can't you just do the below inside this method?

auto ids_of_nodes = ids_of_nodes_optional.value_or_die()

and use ids_of_nodes instead of repeating ids_of_nodes.value()?

[arthurpassos]

Ah, you updated the PR with that here: 46fe767#diff-68aa420a604c13765878c1fb39270f50ae0757b9b2f1b6609743632d2c7d0770R44.

that's ok then

[arthurpassos]

Why reverse?

[ianton-ru]

Change to direct order to avoid confusing.

[arthurpassos]

LGTM

It might be a good idea to add a setting to turn on/ off this behavior just to be on the safe side?

But it looks ok as it is.