Isn't this the same as arangodb_metadata_number_follower_shards.yaml?

Isn't this the same as arangodb_metadata_number_follower_shards.yaml?

I added a separate file for the coordinator metrics as we previously agreed. If you prefer to use the existing 'arangodb_metadata_number_follower_shards.yaml', we need to update it by including 'coordinator' in the 'exposedBy' section.

I guess this file (arangodb_shards_follower_number.yaml) was taken from the PR #22121 (I assume as an example to work with). It can be deleted in this branch, as it will be added when the other one is merged. The same goes for the file arangodb_shard_followers_out_of_sync_number.yaml.

Deleting files.

Please, put 3.12.8 here instead.

Please, put 3.12.8 here instead.

Please, put 3.12.8 here instead.

Please, put 3.12.8 here instead.

Please, put 3.12.8 here instead.

Until now, updateMetadataMetrics() has been the (only) method that updates _metadataMetrics. Now you had the need to add another one to be called during loadCurrent() (updateCoordinatorCurrentMetrics()), and so I also get the sibling updateCoordinatorPlanMetrics() here.

Nevertheless, the current situation with these three methods is a little confusing, at least because updateMetadataMetrics() no longer updates (all the) metadata metrics.

To mitigate that, you could do one or both of

• change the name(s) to clarify their respective responsibilities
• merge updateCoordinatorPlanMetrics() with updateMetadataMetrics()
  .

Later, when we follow through with loading Plan and Current synchronously, it all can be merged into one call.

This case should be impossible, _shards and _shardsToPlanServers have to be consistent here.

It's surely sensible to be defensive and handle it safely regardless. As it hints to a bug at a different place, this should get a maintainer-mode assertion (e.g. TRI_ASSERT(it != _shardsToPlanServers.end() && it->second)). And possibly a log message with an error in production, though that doesn't feel necessary to me.

Fyi, maintainer-mode assertions (TRI_ASSERT) are only compiled in maintainer builds, and are not part of release builds.

As far as I can tell, this case should not be possible, either. Let's also add a maintainer-mode assertion (e.g. TRI_ASSERT(!servers.empty()) to cement this.

This is possibly dangerous, as we're already holding a write lock for _currentProt.lock. We need to be certain this can't introduce deadlocks (e.g. if there is another code path that acquires a write lock for _planProt.lock, and then tries to get a read lock for _currentProt.lock, it could run into a deadlock with this code).

Afair, the ClusterInfo generally only uses one of those locks at a time. But I am not certain, I haven't checked the current code. If there is already code that does that, we must follow a common pattern that avoids deadlocks. And regardless of whether there is such code already, this should be documented at both locks.

Alternatively, we could change the caller to hold only a read lock instead of a write lock for _currentProt.lock: As long as we only ever hold both locks as read locks at the same time, this will prevent deadlocks as well.

I think I prefer the latter solution (only holding read locks), as it also means holding the write lock for a shorter amount of time, and reduce contention with readers: This can be detrimental otherwise. Note that this also holds for not only updateCoordinatorPlanMetrics, but also for the existing function updateMetadataMetrics(): It would be preferable to execute all of them under read locks only! Note that loadPlan and loadCurrent both also hold the _planProt.mutex and _currentProt.mutex, respectively, for their duration: This can be relied on to prevent concurrent calls to the update*Metrics functions.

serversPtr must never be null, or there's a bug somewhere else. Double-checking it is absolutely fine, but I'd also add a maintainer-mode assertion to spot possible bugs during testing (e.g. TRI_ASSERT(serversPtr != nullptr)).

Similarly: In addition to serversPtr not being null, I think *serversPtr must never be empty. I'd also add a maintainer-mode assertion to that end. Though in this case, I am not 100% certain, so please do keep the check. :-)

Note that currently, Current can have stale entries, i.e. for collections/shards that have already been deleted. This could cause miscounts. It may help to think of the Plan as the actual cluster-wide metadata for e.g. a collection, and corresponding Current entries as a last reported state.

I suggest to flip it around: Loop over _shardsToPlanServers instead, and look up the corresponding entries in _shardsToCurrent.

These two metrics should only be updated in updateCoordinatorCurrentMetrics():

Which also means we don't need to calculate notReplicated here (and we can't, really).

These two metrics are already being reported by updateCoordinatorCurrentMetrics(), and should not be reported here as well.

Imo outOfSync and notReplicated don't quite count the right thing; let me try to explain my idea of them (high-level, skipping over some details):

If there is only one in-sync replica (the leader), the shard should be counted as notReplicated. This is also the case for shards that are planned with replicationFactor: 1 (which means, the plan already only contains the leader as the only replica).

If any follower from Plan is missing from the list in Current, it is regarded as out-of-sync.

Note that a shard can be counted as both notReplicated and outOfSync, or any one of them, or neither.

But note that there are some additional edge cases to consider, e.g. during a leader failover or move shard operation. Let's discuss these separately.