SOLR-18538: CREATESNAPSHOT reports success when a shard has no active replica

[SpookyJumpyBeans]

https://issues.apache.org/jira/browse/SOLR-18538

Description

CreateSnapshotCmd skips replicas that are not ACTIVE and only evaluates failures for shards it sent a core snapshot request to. If every replica of a shard is inactive, that shard is never checked and the collection snapshot is recorded as Successful with no core snapshot for it.

The same happens when a shard's only active replica fails, because the failure count was compared against all of the shard's replicas, including the inactive ones that were skipped. In both cases the snapshot is reported as successful even though it can't be used to back up the missing shard.

Solution

Replace the per-shard failure counting with the check the existing comment describes: after collecting the successful core snapshots, every active slice must have at least one replica that reported a snapshot.

Set<String> shardsWithSnapshot = new HashSet<>();
for (CoreSnapshotMetaData replica : replicas) {
  shardsWithSnapshot.add(replica.getShardId());
}
for (Slice slice : collectionStatus.getActiveSlices()) {
  if (!shardsWithSnapshot.contains(slice.getName())) {
    failedShards.add(slice.getName());
  }
}

Any active slice without a snapshot is added to failedShards, so the command records Failed and throws, as it already did for other failures.

Tests

Added TestSolrCloudSnapshotInactiveShard, which creates a single-shard, single-replica collection, stops the only node hosting it, and checks that CREATESNAPSHOT fails with "Failed to create snapshot on shards" and the snapshot is recorded as Failed with no core snapshots. The test fails without the fix (the request succeeds) and passes with it.

The second case (a shard whose only active replica fails) goes through the same new check, but isn't covered by a separate test, since making a single core snapshot fail on demand isn't straightforward in a cloud test. Happy to add one if there's a preferred way to inject that failure.

Also ran the existing snapshot tests (TestSolrCloudSnapshots with -Ptests.nightly=true, TestSolrCoreSnapshots, CreateCollectionSnapshotAPITest) and ./gradlew :solr:core:check -x test.

Checklist

• I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
• I have created a Jira issue and added the issue ID to my pull request title.
• I have given Solr maintainers access to contribute to my PR branch. (optional but recommended, not available for branches on forks living under an organisation)
• I have developed this patch against the main branch.
• I have run ./gradlew check.
• I have added tests for my changes.
• I have added documentation for the Reference Guide
• I have added a changelog entry for my change

[psalagnac]

Is there any good reason for codec suppression? This test does not seem to depend on it.

[SpookyJumpyBeans]

Good catch, there’s no reason for it. I copied it from TestSolrCloudSnapshots, which needs it for backups, but this test doesn’t index or back up anything. Removed.

[psalagnac]

We iterate only over active slices, while the initial loop to get all replicas (near line 102) considers all slices, not just active ones.

This seems inconsistent. If we don't consider the non active shards as part of the success/failure criteria, we could skip them initially.

[SpookyJumpyBeans]

Agreed. BackupCmd only uses active slices anyway, so there’s no reason to snapshot inactive ones. The request loop now iterates getActiveSlices() too, so both steps use the same set.

[psalagnac]

I'd rather we don't read the collection state a second time. We should use same version we previously read. Second call is done after processResponses(), which is blocking and can take a while. The collection state might have slightly changed.

The old code had the same double read, but it only ran on the failure path.

[SpookyJumpyBeans]

Makes sense. The collection state is now read once before sending the requests, and the same DocCollection is used for the success check, so it can’t change between the two.