Store clusters from ClusterDiscovery in separate map

[vdimir]

Changelog category (leave one):

• Not for changelog (changelog entry is not required)

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

Ref #47176

Context:

Clusters defined in discovery mode were stored in shared->clusters with others. But the variable shared->clusters is assigned in several places (e.g., on config reloading), and it's challenging to maintain it in the correct state. Current PR moves data structure to store auto-discovered clusters out of shared->clusters, similar to how it's done for the replicated database.

[antonio2368]

oh no, you take a lock each time and possibly get a different cluster map each time?

[vdimir]

Inside shared->cluster_discovery, we have a mutex to protect cluster_impls. It can be updated anytime (on the server added to/removed from the cluster).
As for shared->cluster_discovery itself, it initialized once at Context::startClusterDiscovery, but maybe it's worth protecting it with shared->clusters_mutex also (as in tryGetCluster), because I'm not really sure that getClusters can't be called during server startup.

[antonio2368]

It can be updated anytime (on the server added to/removed from the cluster).

Yes, but that's a problem when iterating no?
Each loop iteration you will end up with different possible iterators if map changes.

[vdimir]

I assumed that

for (const auto & [name, cluster] : shared->cluster_discovery->getClusters())
{
...

is the same as

const auto & tmp = shared->cluster_discovery->getClusters();
for (const auto & [name, cluster] : tmp)
{
...

and getClusters is just called once.

https://godbolt.org/z/sj6Ydc7zT

But we can change the code to be like the second snippet to ensure it.

Suggested change

	for (const auto & [name, cluster] : shared->cluster_discovery->getClusters())
	const auto & cluster_discovery_map = shared->cluster_discovery->getClusters();
	for (const auto & [name, cluster] : cluster_discovery_map)

[antonio2368]

there is a cool tool for things like this, C++ Insights
your example

But yeah, I'm still afraid to have code that relies on such assumptions because it's not obvious

[vdimir]

Rerun CI to check integration tests broken by bad docker image