From 1fae3e75017ab831eab5ff30029bc97ebd52fe82 Mon Sep 17 00:00:00 2001 From: David Turner Date: Wed, 6 Mar 2024 15:27:57 +0000 Subject: [PATCH] Extract `SnapshotSortKey` (#106015) The behaviour of the get-snapshots API varies quite considerably depending on the sort key chosen. Today this logic is implemented using scattered `switch` statements and other conditionals but it'd be clearer if we delegated this stuff to the sort key instances themselves. This commit moves the sort key enum to the top level and replaces one of the `switch` statements with a method on the enum instances. --- .../http/snapshots/RestGetSnapshotsIT.java | 70 ++++----- .../snapshots/GetSnapshotsIT.java | 139 +++++++----------- .../snapshots/get/GetSnapshotsRequest.java | 46 +----- .../get/GetSnapshotsRequestBuilder.java | 2 +- .../snapshots/get/SnapshotSortKey.java | 83 +++++++++++ .../get/TransportGetSnapshotsAction.java | 39 +---- .../admin/cluster/RestGetSnapshotsAction.java | 3 +- .../get/GetSnapshotsRequestTests.java | 3 +- .../AbstractSnapshotIntegTestCase.java | 8 +- 9 files changed, 191 insertions(+), 202 deletions(-) create mode 100644 server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/SnapshotSortKey.java diff --git a/qa/smoke-test-http/src/javaRestTest/java/org/elasticsearch/http/snapshots/RestGetSnapshotsIT.java b/qa/smoke-test-http/src/javaRestTest/java/org/elasticsearch/http/snapshots/RestGetSnapshotsIT.java index e9f410643377..59e07581499e 100644 --- a/qa/smoke-test-http/src/javaRestTest/java/org/elasticsearch/http/snapshots/RestGetSnapshotsIT.java +++ b/qa/smoke-test-http/src/javaRestTest/java/org/elasticsearch/http/snapshots/RestGetSnapshotsIT.java @@ -14,6 +14,7 @@ import org.elasticsearch.action.ActionFuture; import org.elasticsearch.action.admin.cluster.snapshots.create.CreateSnapshotResponse; import org.elasticsearch.action.admin.cluster.snapshots.get.GetSnapshotsRequest; import org.elasticsearch.action.admin.cluster.snapshots.get.GetSnapshotsResponse; +import org.elasticsearch.action.admin.cluster.snapshots.get.SnapshotSortKey; import org.elasticsearch.client.Request; import org.elasticsearch.client.Response; import org.elasticsearch.cluster.SnapshotsInProgress; @@ -101,38 +102,38 @@ public class RestGetSnapshotsIT extends AbstractSnapshotRestTestCase { .getSnapshots(); assertSnapshotListSorted(defaultSorting, null, order); assertSnapshotListSorted( - allSnapshotsSorted(allSnapshotNames, repoName, GetSnapshotsRequest.SortBy.NAME, order, includeIndexNames), - GetSnapshotsRequest.SortBy.NAME, + allSnapshotsSorted(allSnapshotNames, repoName, SnapshotSortKey.NAME, order, includeIndexNames), + SnapshotSortKey.NAME, order ); assertSnapshotListSorted( - allSnapshotsSorted(allSnapshotNames, repoName, GetSnapshotsRequest.SortBy.DURATION, order, includeIndexNames), - GetSnapshotsRequest.SortBy.DURATION, + allSnapshotsSorted(allSnapshotNames, repoName, SnapshotSortKey.DURATION, order, includeIndexNames), + SnapshotSortKey.DURATION, order ); assertSnapshotListSorted( - allSnapshotsSorted(allSnapshotNames, repoName, GetSnapshotsRequest.SortBy.INDICES, order, includeIndexNames), - GetSnapshotsRequest.SortBy.INDICES, + allSnapshotsSorted(allSnapshotNames, repoName, SnapshotSortKey.INDICES, order, includeIndexNames), + SnapshotSortKey.INDICES, order ); assertSnapshotListSorted( - allSnapshotsSorted(allSnapshotNames, repoName, GetSnapshotsRequest.SortBy.START_TIME, order, includeIndexNames), - GetSnapshotsRequest.SortBy.START_TIME, + allSnapshotsSorted(allSnapshotNames, repoName, SnapshotSortKey.START_TIME, order, includeIndexNames), + SnapshotSortKey.START_TIME, order ); assertSnapshotListSorted( - allSnapshotsSorted(allSnapshotNames, repoName, GetSnapshotsRequest.SortBy.SHARDS, order, includeIndexNames), - GetSnapshotsRequest.SortBy.SHARDS, + allSnapshotsSorted(allSnapshotNames, repoName, SnapshotSortKey.SHARDS, order, includeIndexNames), + SnapshotSortKey.SHARDS, order ); assertSnapshotListSorted( - allSnapshotsSorted(allSnapshotNames, repoName, GetSnapshotsRequest.SortBy.FAILED_SHARDS, order, includeIndexNames), - GetSnapshotsRequest.SortBy.FAILED_SHARDS, + allSnapshotsSorted(allSnapshotNames, repoName, SnapshotSortKey.FAILED_SHARDS, order, includeIndexNames), + SnapshotSortKey.FAILED_SHARDS, order ); assertSnapshotListSorted( - allSnapshotsSorted(allSnapshotNames, repoName, GetSnapshotsRequest.SortBy.REPOSITORY, order, includeIndexNames), - GetSnapshotsRequest.SortBy.REPOSITORY, + allSnapshotsSorted(allSnapshotNames, repoName, SnapshotSortKey.REPOSITORY, order, includeIndexNames), + SnapshotSortKey.REPOSITORY, order ); } @@ -141,7 +142,7 @@ public class RestGetSnapshotsIT extends AbstractSnapshotRestTestCase { final String repoName = "test-repo"; AbstractSnapshotIntegTestCase.createRepository(logger, repoName, "fs"); final List names = AbstractSnapshotIntegTestCase.createNSnapshots(logger, repoName, randomIntBetween(6, 20)); - for (GetSnapshotsRequest.SortBy sort : GetSnapshotsRequest.SortBy.values()) { + for (SnapshotSortKey sort : SnapshotSortKey.values()) { for (SortOrder order : SortOrder.values()) { logger.info("--> testing pagination for [{}] [{}]", sort, order); doTestPagination(repoName, names, sort, order); @@ -149,8 +150,7 @@ public class RestGetSnapshotsIT extends AbstractSnapshotRestTestCase { } } - private void doTestPagination(String repoName, List names, GetSnapshotsRequest.SortBy sort, SortOrder order) - throws IOException { + private void doTestPagination(String repoName, List names, SnapshotSortKey sort, SortOrder order) throws IOException { final boolean includeIndexNames = randomBoolean(); final List allSnapshotsSorted = allSnapshotsSorted(names, repoName, sort, order, includeIndexNames); final GetSnapshotsResponse batch1 = sortedWithLimit(repoName, sort, null, 2, order, includeIndexNames); @@ -220,18 +220,18 @@ public class RestGetSnapshotsIT extends AbstractSnapshotRestTestCase { .equals(Map.of(SnapshotsInProgress.ShardState.INIT, 1L, SnapshotsInProgress.ShardState.QUEUED, (long) inProgressCount - 1)); return firstIndexSuccessfullySnapshot && secondIndexIsBlocked; }); - assertStablePagination(repoName, allSnapshotNames, GetSnapshotsRequest.SortBy.START_TIME); - assertStablePagination(repoName, allSnapshotNames, GetSnapshotsRequest.SortBy.NAME); - assertStablePagination(repoName, allSnapshotNames, GetSnapshotsRequest.SortBy.INDICES); + assertStablePagination(repoName, allSnapshotNames, SnapshotSortKey.START_TIME); + assertStablePagination(repoName, allSnapshotNames, SnapshotSortKey.NAME); + assertStablePagination(repoName, allSnapshotNames, SnapshotSortKey.INDICES); AbstractSnapshotIntegTestCase.unblockAllDataNodes(repoName); for (ActionFuture inProgressSnapshot : inProgressSnapshots) { AbstractSnapshotIntegTestCase.assertSuccessful(logger, inProgressSnapshot); } - assertStablePagination(repoName, allSnapshotNames, GetSnapshotsRequest.SortBy.START_TIME); - assertStablePagination(repoName, allSnapshotNames, GetSnapshotsRequest.SortBy.NAME); - assertStablePagination(repoName, allSnapshotNames, GetSnapshotsRequest.SortBy.INDICES); + assertStablePagination(repoName, allSnapshotNames, SnapshotSortKey.START_TIME); + assertStablePagination(repoName, allSnapshotNames, SnapshotSortKey.NAME); + assertStablePagination(repoName, allSnapshotNames, SnapshotSortKey.INDICES); } public void testFilterBySLMPolicy() throws Exception { @@ -240,7 +240,7 @@ public class RestGetSnapshotsIT extends AbstractSnapshotRestTestCase { AbstractSnapshotIntegTestCase.createNSnapshots(logger, repoName, randomIntBetween(1, 5)); final List snapshotsWithoutPolicy = clusterAdmin().prepareGetSnapshots("*") .setSnapshots("*") - .setSort(GetSnapshotsRequest.SortBy.NAME) + .setSort(SnapshotSortKey.NAME) .get() .getSnapshots(); final String snapshotWithPolicy = "snapshot-with-policy"; @@ -277,7 +277,7 @@ public class RestGetSnapshotsIT extends AbstractSnapshotRestTestCase { assertThat(getAllSnapshotsForPolicies(policyName, otherPolicyName, "no-such-policy*"), is(List.of(withOtherPolicy, withPolicy))); final List allSnapshots = clusterAdmin().prepareGetSnapshots("*") .setSnapshots("*") - .setSort(GetSnapshotsRequest.SortBy.NAME) + .setSort(SnapshotSortKey.NAME) .get() .getSnapshots(); assertThat(getAllSnapshotsForPolicies(GetSnapshotsRequest.NO_POLICY_PATTERN, policyName, otherPolicyName), is(allSnapshots)); @@ -294,7 +294,7 @@ public class RestGetSnapshotsIT extends AbstractSnapshotRestTestCase { final List allSnapshotInfo = clusterAdmin().prepareGetSnapshots(matchAllPattern()) .setSnapshots(matchAllPattern()) - .setSort(GetSnapshotsRequest.SortBy.START_TIME) + .setSort(SnapshotSortKey.START_TIME) .get() .getSnapshots(); assertThat(allSnapshotInfo, is(List.of(snapshot1, snapshot2, snapshot3))); @@ -311,7 +311,7 @@ public class RestGetSnapshotsIT extends AbstractSnapshotRestTestCase { final List allSnapshotInfoDesc = clusterAdmin().prepareGetSnapshots(matchAllPattern()) .setSnapshots(matchAllPattern()) - .setSort(GetSnapshotsRequest.SortBy.START_TIME) + .setSort(SnapshotSortKey.START_TIME) .setOrder(SortOrder.DESC) .get() .getSnapshots(); @@ -340,7 +340,7 @@ public class RestGetSnapshotsIT extends AbstractSnapshotRestTestCase { private List allAfterStartTimeAscending(long timestamp) throws IOException { final Request request = baseGetSnapshotsRequest("*"); - request.addParameter("sort", GetSnapshotsRequest.SortBy.START_TIME.toString()); + request.addParameter("sort", SnapshotSortKey.START_TIME.toString()); request.addParameter("from_sort_value", String.valueOf(timestamp)); final Response response = getRestClient().performRequest(request); return readSnapshotInfos(response).getSnapshots(); @@ -348,7 +348,7 @@ public class RestGetSnapshotsIT extends AbstractSnapshotRestTestCase { private List allBeforeStartTimeDescending(long timestamp) throws IOException { final Request request = baseGetSnapshotsRequest("*"); - request.addParameter("sort", GetSnapshotsRequest.SortBy.START_TIME.toString()); + request.addParameter("sort", SnapshotSortKey.START_TIME.toString()); request.addParameter("from_sort_value", String.valueOf(timestamp)); request.addParameter("order", SortOrder.DESC.toString()); final Response response = getRestClient().performRequest(request); @@ -358,7 +358,7 @@ public class RestGetSnapshotsIT extends AbstractSnapshotRestTestCase { private static List getAllSnapshotsForPolicies(String... policies) throws IOException { final Request requestWithPolicy = new Request(HttpGet.METHOD_NAME, "/_snapshot/*/*"); requestWithPolicy.addParameter("slm_policy_filter", Strings.arrayToCommaDelimitedString(policies)); - requestWithPolicy.addParameter("sort", GetSnapshotsRequest.SortBy.NAME.toString()); + requestWithPolicy.addParameter("sort", SnapshotSortKey.NAME.toString()); return readSnapshotInfos(getRestClient().performRequest(requestWithPolicy)).getSnapshots(); } @@ -369,10 +369,10 @@ public class RestGetSnapshotsIT extends AbstractSnapshotRestTestCase { indexDoc(indexName, "some_id", "foo", "bar"); } - private static void assertStablePagination(String repoName, Collection allSnapshotNames, GetSnapshotsRequest.SortBy sort) + private static void assertStablePagination(String repoName, Collection allSnapshotNames, SnapshotSortKey sort) throws IOException { final SortOrder order = randomFrom(SortOrder.values()); - final boolean includeIndexNames = sort == GetSnapshotsRequest.SortBy.INDICES || randomBoolean(); + final boolean includeIndexNames = sort == SnapshotSortKey.INDICES || randomBoolean(); final List allSorted = allSnapshotsSorted(allSnapshotNames, repoName, sort, order, includeIndexNames); for (int i = 1; i <= allSnapshotNames.size(); i++) { @@ -413,7 +413,7 @@ public class RestGetSnapshotsIT extends AbstractSnapshotRestTestCase { private static List allSnapshotsSorted( Collection allSnapshotNames, String repoName, - GetSnapshotsRequest.SortBy sortBy, + SnapshotSortKey sortBy, SortOrder order, boolean includeIndices ) throws IOException { @@ -454,7 +454,7 @@ public class RestGetSnapshotsIT extends AbstractSnapshotRestTestCase { private static GetSnapshotsResponse sortedWithLimit( String repoName, - GetSnapshotsRequest.SortBy sortBy, + SnapshotSortKey sortBy, String after, int size, SortOrder order, @@ -486,7 +486,7 @@ public class RestGetSnapshotsIT extends AbstractSnapshotRestTestCase { private static GetSnapshotsResponse sortedWithLimit( String repoName, - GetSnapshotsRequest.SortBy sortBy, + SnapshotSortKey sortBy, int offset, int size, SortOrder order, diff --git a/server/src/internalClusterTest/java/org/elasticsearch/snapshots/GetSnapshotsIT.java b/server/src/internalClusterTest/java/org/elasticsearch/snapshots/GetSnapshotsIT.java index 6b5b3826272c..d01064b9fb8b 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/snapshots/GetSnapshotsIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/snapshots/GetSnapshotsIT.java @@ -14,6 +14,7 @@ import org.elasticsearch.action.admin.cluster.snapshots.create.CreateSnapshotRes import org.elasticsearch.action.admin.cluster.snapshots.get.GetSnapshotsRequest; import org.elasticsearch.action.admin.cluster.snapshots.get.GetSnapshotsRequestBuilder; import org.elasticsearch.action.admin.cluster.snapshots.get.GetSnapshotsResponse; +import org.elasticsearch.action.admin.cluster.snapshots.get.SnapshotSortKey; import org.elasticsearch.cluster.SnapshotsInProgress; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.repositories.RepositoryMissingException; @@ -84,39 +85,31 @@ public class GetSnapshotsIT extends AbstractSnapshotIntegTestCase { final List defaultSorting = clusterAdmin().prepareGetSnapshots(repoName).setOrder(order).get().getSnapshots(); assertSnapshotListSorted(defaultSorting, null, order); final String[] repos = { repoName }; + assertSnapshotListSorted(allSnapshotsSorted(allSnapshotNames, repos, SnapshotSortKey.NAME, order), SnapshotSortKey.NAME, order); assertSnapshotListSorted( - allSnapshotsSorted(allSnapshotNames, repos, GetSnapshotsRequest.SortBy.NAME, order), - GetSnapshotsRequest.SortBy.NAME, + allSnapshotsSorted(allSnapshotNames, repos, SnapshotSortKey.DURATION, order), + SnapshotSortKey.DURATION, order ); assertSnapshotListSorted( - allSnapshotsSorted(allSnapshotNames, repos, GetSnapshotsRequest.SortBy.DURATION, order), - GetSnapshotsRequest.SortBy.DURATION, + allSnapshotsSorted(allSnapshotNames, repos, SnapshotSortKey.INDICES, order), + SnapshotSortKey.INDICES, order ); assertSnapshotListSorted( - allSnapshotsSorted(allSnapshotNames, repos, GetSnapshotsRequest.SortBy.INDICES, order), - GetSnapshotsRequest.SortBy.INDICES, + allSnapshotsSorted(allSnapshotNames, repos, SnapshotSortKey.START_TIME, order), + SnapshotSortKey.START_TIME, + order + ); + assertSnapshotListSorted(allSnapshotsSorted(allSnapshotNames, repos, SnapshotSortKey.SHARDS, order), SnapshotSortKey.SHARDS, order); + assertSnapshotListSorted( + allSnapshotsSorted(allSnapshotNames, repos, SnapshotSortKey.FAILED_SHARDS, order), + SnapshotSortKey.FAILED_SHARDS, order ); assertSnapshotListSorted( - allSnapshotsSorted(allSnapshotNames, repos, GetSnapshotsRequest.SortBy.START_TIME, order), - GetSnapshotsRequest.SortBy.START_TIME, - order - ); - assertSnapshotListSorted( - allSnapshotsSorted(allSnapshotNames, repos, GetSnapshotsRequest.SortBy.SHARDS, order), - GetSnapshotsRequest.SortBy.SHARDS, - order - ); - assertSnapshotListSorted( - allSnapshotsSorted(allSnapshotNames, repos, GetSnapshotsRequest.SortBy.FAILED_SHARDS, order), - GetSnapshotsRequest.SortBy.FAILED_SHARDS, - order - ); - assertSnapshotListSorted( - allSnapshotsSorted(allSnapshotNames, repos, GetSnapshotsRequest.SortBy.REPOSITORY, order), - GetSnapshotsRequest.SortBy.REPOSITORY, + allSnapshotsSorted(allSnapshotNames, repos, SnapshotSortKey.REPOSITORY, order), + SnapshotSortKey.REPOSITORY, order ); } @@ -127,7 +120,7 @@ public class GetSnapshotsIT extends AbstractSnapshotIntegTestCase { createRepository(repoName, "fs", repoPath); maybeInitWithOldSnapshotVersion(repoName, repoPath); final List names = createNSnapshots(repoName, randomIntBetween(6, 20)); - for (GetSnapshotsRequest.SortBy sort : GetSnapshotsRequest.SortBy.values()) { + for (SnapshotSortKey sort : SnapshotSortKey.values()) { for (SortOrder order : SortOrder.values()) { logger.info("--> testing pagination for [{}] [{}]", sort, order); doTestPagination(repoName, names, sort, order); @@ -135,7 +128,7 @@ public class GetSnapshotsIT extends AbstractSnapshotIntegTestCase { } } - private void doTestPagination(String repoName, List names, GetSnapshotsRequest.SortBy sort, SortOrder order) { + private void doTestPagination(String repoName, List names, SnapshotSortKey sort, SortOrder order) { final String[] repos = { repoName }; final List allSnapshotsSorted = allSnapshotsSorted(names, repos, sort, order); final GetSnapshotsResponse batch1 = sortedWithLimit(repos, sort, null, 2, order); @@ -191,9 +184,9 @@ public class GetSnapshotsIT extends AbstractSnapshotIntegTestCase { ) ); final String[] repos = { repoName }; - assertStablePagination(repos, allSnapshotNames, GetSnapshotsRequest.SortBy.START_TIME); - assertStablePagination(repos, allSnapshotNames, GetSnapshotsRequest.SortBy.NAME); - assertStablePagination(repos, allSnapshotNames, GetSnapshotsRequest.SortBy.INDICES); + assertStablePagination(repos, allSnapshotNames, SnapshotSortKey.START_TIME); + assertStablePagination(repos, allSnapshotNames, SnapshotSortKey.NAME); + assertStablePagination(repos, allSnapshotNames, SnapshotSortKey.INDICES); final List currentSnapshots = clusterAdmin().prepareGetSnapshots(matchAllPattern()) .setSnapshots(GetSnapshotsRequest.CURRENT_SNAPSHOT) .get() @@ -215,9 +208,9 @@ public class GetSnapshotsIT extends AbstractSnapshotIntegTestCase { assertSuccessful(inProgressSnapshot); } - assertStablePagination(repos, allSnapshotNames, GetSnapshotsRequest.SortBy.START_TIME); - assertStablePagination(repos, allSnapshotNames, GetSnapshotsRequest.SortBy.NAME); - assertStablePagination(repos, allSnapshotNames, GetSnapshotsRequest.SortBy.INDICES); + assertStablePagination(repos, allSnapshotNames, SnapshotSortKey.START_TIME); + assertStablePagination(repos, allSnapshotNames, SnapshotSortKey.NAME); + assertStablePagination(repos, allSnapshotNames, SnapshotSortKey.INDICES); } public void testPaginationRequiresVerboseListing() throws Exception { @@ -228,14 +221,14 @@ public class GetSnapshotsIT extends AbstractSnapshotIntegTestCase { ActionRequestValidationException.class, clusterAdmin().prepareGetSnapshots(repoName) .setVerbose(false) - .setSort(GetSnapshotsRequest.SortBy.DURATION) + .setSort(SnapshotSortKey.DURATION) .setSize(GetSnapshotsRequest.NO_LIMIT) ); expectThrows( ActionRequestValidationException.class, clusterAdmin().prepareGetSnapshots(repoName) .setVerbose(false) - .setSort(GetSnapshotsRequest.SortBy.START_TIME) + .setSort(SnapshotSortKey.START_TIME) .setSize(randomIntBetween(1, 100)) ); } @@ -258,16 +251,11 @@ public class GetSnapshotsIT extends AbstractSnapshotIntegTestCase { allSnapshotNames.addAll(namesOtherRepo); final SortOrder order = SortOrder.DESC; - final List allSorted = allSnapshotsSorted( - allSnapshotNames, - new String[] { "*" }, - GetSnapshotsRequest.SortBy.REPOSITORY, - order - ); + final List allSorted = allSnapshotsSorted(allSnapshotNames, new String[] { "*" }, SnapshotSortKey.REPOSITORY, order); final List allSortedWithoutOther = allSnapshotsSorted( allSnapshotNamesWithoutOther, new String[] { "*", "-" + otherRepo }, - GetSnapshotsRequest.SortBy.REPOSITORY, + SnapshotSortKey.REPOSITORY, order ); assertThat(allSortedWithoutOther, is(allSorted.subList(0, allSnapshotNamesWithoutOther.size()))); @@ -275,7 +263,7 @@ public class GetSnapshotsIT extends AbstractSnapshotIntegTestCase { final List allInOther = allSnapshotsSorted( namesOtherRepo, new String[] { "*", "-test-repo-*" }, - GetSnapshotsRequest.SortBy.REPOSITORY, + SnapshotSortKey.REPOSITORY, order ); assertThat(allInOther, is(allSorted.subList(allSnapshotNamesWithoutOther.size(), allSorted.size()))); @@ -289,7 +277,7 @@ public class GetSnapshotsIT extends AbstractSnapshotIntegTestCase { final List allInOtherWithoutOtherPrefix = allSnapshotsSorted( namesOtherRepo, patternOtherRepo, - GetSnapshotsRequest.SortBy.REPOSITORY, + SnapshotSortKey.REPOSITORY, order, "-other*" ); @@ -298,7 +286,7 @@ public class GetSnapshotsIT extends AbstractSnapshotIntegTestCase { final List allInOtherWithoutOtherExplicit = allSnapshotsSorted( namesOtherRepo, patternOtherRepo, - GetSnapshotsRequest.SortBy.REPOSITORY, + SnapshotSortKey.REPOSITORY, order, "-" + otherPrefixSnapshot1, "-" + otherPrefixSnapshot2 @@ -345,7 +333,7 @@ public class GetSnapshotsIT extends AbstractSnapshotIntegTestCase { final SnapshotInfo weirdSnapshot2InWeird2 = createFullSnapshot(weirdRepo2, weirdSnapshot2); final List allSnapshots = clusterAdmin().prepareGetSnapshots(matchAllPattern()) - .setSort(GetSnapshotsRequest.SortBy.REPOSITORY) + .setSort(SnapshotSortKey.REPOSITORY) .get() .getSnapshots(); assertThat(allSnapshots, hasSize(9)); @@ -407,11 +395,7 @@ public class GetSnapshotsIT extends AbstractSnapshotIntegTestCase { } private List getAllByPatterns(String[] repos, String[] snapshots) { - return clusterAdmin().prepareGetSnapshots(repos) - .setSnapshots(snapshots) - .setSort(GetSnapshotsRequest.SortBy.REPOSITORY) - .get() - .getSnapshots(); + return clusterAdmin().prepareGetSnapshots(repos).setSnapshots(snapshots).setSort(SnapshotSortKey.REPOSITORY).get().getSnapshots(); } public void testFilterBySLMPolicy() throws Exception { @@ -420,7 +404,7 @@ public class GetSnapshotsIT extends AbstractSnapshotIntegTestCase { createNSnapshots(repoName, randomIntBetween(1, 5)); final List snapshotsWithoutPolicy = clusterAdmin().prepareGetSnapshots(matchAllPattern()) .setSnapshots(matchAllPattern()) - .setSort(GetSnapshotsRequest.SortBy.NAME) + .setSort(SnapshotSortKey.NAME) .get() .getSnapshots(); final String snapshotWithPolicy = "snapshot-with-policy"; @@ -456,7 +440,7 @@ public class GetSnapshotsIT extends AbstractSnapshotIntegTestCase { final List allSnapshots = clusterAdmin().prepareGetSnapshots(matchAllPattern()) .setSnapshots(matchAllPattern()) - .setSort(GetSnapshotsRequest.SortBy.NAME) + .setSort(SnapshotSortKey.NAME) .get() .getSnapshots(); assertThat(getAllSnapshotsForPolicies(GetSnapshotsRequest.NO_POLICY_PATTERN, policyName, otherPolicyName), is(allSnapshots)); @@ -477,7 +461,7 @@ public class GetSnapshotsIT extends AbstractSnapshotIntegTestCase { final List allSnapshotInfo = clusterAdmin().prepareGetSnapshots(matchAllPattern()) .setSnapshots(matchAllPattern()) - .setSort(GetSnapshotsRequest.SortBy.START_TIME) + .setSort(SnapshotSortKey.START_TIME) .get() .getSnapshots(); assertThat(allSnapshotInfo, is(List.of(snapshot1, snapshot2, snapshot3))); @@ -504,7 +488,7 @@ public class GetSnapshotsIT extends AbstractSnapshotIntegTestCase { final List allSnapshotInfoDesc = clusterAdmin().prepareGetSnapshots(matchAllPattern()) .setSnapshots(matchAllPattern()) - .setSort(GetSnapshotsRequest.SortBy.START_TIME) + .setSort(SnapshotSortKey.START_TIME) .setOrder(SortOrder.DESC) .get() .getSnapshots(); @@ -525,7 +509,7 @@ public class GetSnapshotsIT extends AbstractSnapshotIntegTestCase { final List allSnapshotInfoByDuration = clusterAdmin().prepareGetSnapshots(matchAllPattern()) .setSnapshots(matchAllPattern()) - .setSort(GetSnapshotsRequest.SortBy.DURATION) + .setSort(SnapshotSortKey.DURATION) .get() .getSnapshots(); @@ -541,7 +525,7 @@ public class GetSnapshotsIT extends AbstractSnapshotIntegTestCase { final List allSnapshotInfoByDurationDesc = clusterAdmin().prepareGetSnapshots(matchAllPattern()) .setSnapshots(matchAllPattern()) - .setSort(GetSnapshotsRequest.SortBy.DURATION) + .setSort(SnapshotSortKey.DURATION) .setOrder(SortOrder.DESC) .get() .getSnapshots(); @@ -554,12 +538,12 @@ public class GetSnapshotsIT extends AbstractSnapshotIntegTestCase { final SnapshotInfo otherSnapshot = createFullSnapshot(repoName, "other-snapshot"); - assertThat(allSnapshots(new String[] { "snap*" }, GetSnapshotsRequest.SortBy.NAME, SortOrder.ASC, "a"), is(allSnapshotInfo)); - assertThat(allSnapshots(new String[] { "o*" }, GetSnapshotsRequest.SortBy.NAME, SortOrder.ASC, "a"), is(List.of(otherSnapshot))); + assertThat(allSnapshots(new String[] { "snap*" }, SnapshotSortKey.NAME, SortOrder.ASC, "a"), is(allSnapshotInfo)); + assertThat(allSnapshots(new String[] { "o*" }, SnapshotSortKey.NAME, SortOrder.ASC, "a"), is(List.of(otherSnapshot))); final GetSnapshotsResponse paginatedResponse = clusterAdmin().prepareGetSnapshots(matchAllPattern()) .setSnapshots("snap*") - .setSort(GetSnapshotsRequest.SortBy.NAME) + .setSort(SnapshotSortKey.NAME) .setFromSortValue("a") .setOffset(1) .setSize(1) @@ -568,7 +552,7 @@ public class GetSnapshotsIT extends AbstractSnapshotIntegTestCase { assertThat(paginatedResponse.totalCount(), is(3)); final GetSnapshotsResponse paginatedResponse2 = clusterAdmin().prepareGetSnapshots(matchAllPattern()) .setSnapshots("snap*") - .setSort(GetSnapshotsRequest.SortBy.NAME) + .setSort(SnapshotSortKey.NAME) .setFromSortValue("a") .setOffset(0) .setSize(2) @@ -587,7 +571,7 @@ public class GetSnapshotsIT extends AbstractSnapshotIntegTestCase { snapshotNames.sort(String::compareTo); final GetSnapshotsResponse response = clusterAdmin().prepareGetSnapshots(repoName, missingRepoName) - .setSort(GetSnapshotsRequest.SortBy.NAME) + .setSort(SnapshotSortKey.NAME) .get(); assertThat(response.getSnapshots().stream().map(info -> info.snapshotId().getName()).toList(), equalTo(snapshotNames)); assertTrue(response.getFailures().containsKey(missingRepoName)); @@ -618,35 +602,30 @@ public class GetSnapshotsIT extends AbstractSnapshotIntegTestCase { } private List allAfterStartTimeAscending(long timestamp) { - return allSnapshots(matchAllPattern(), GetSnapshotsRequest.SortBy.START_TIME, SortOrder.ASC, timestamp); + return allSnapshots(matchAllPattern(), SnapshotSortKey.START_TIME, SortOrder.ASC, timestamp); } private List allBeforeStartTimeDescending(long timestamp) { - return allSnapshots(matchAllPattern(), GetSnapshotsRequest.SortBy.START_TIME, SortOrder.DESC, timestamp); + return allSnapshots(matchAllPattern(), SnapshotSortKey.START_TIME, SortOrder.DESC, timestamp); } private List allAfterNameAscending(String name) { - return allSnapshots(matchAllPattern(), GetSnapshotsRequest.SortBy.NAME, SortOrder.ASC, name); + return allSnapshots(matchAllPattern(), SnapshotSortKey.NAME, SortOrder.ASC, name); } private List allBeforeNameDescending(String name) { - return allSnapshots(matchAllPattern(), GetSnapshotsRequest.SortBy.NAME, SortOrder.DESC, name); + return allSnapshots(matchAllPattern(), SnapshotSortKey.NAME, SortOrder.DESC, name); } private List allAfterDurationAscending(long duration) { - return allSnapshots(matchAllPattern(), GetSnapshotsRequest.SortBy.DURATION, SortOrder.ASC, duration); + return allSnapshots(matchAllPattern(), SnapshotSortKey.DURATION, SortOrder.ASC, duration); } private List allBeforeDurationDescending(long duration) { - return allSnapshots(matchAllPattern(), GetSnapshotsRequest.SortBy.DURATION, SortOrder.DESC, duration); + return allSnapshots(matchAllPattern(), SnapshotSortKey.DURATION, SortOrder.DESC, duration); } - private static List allSnapshots( - String[] snapshotNames, - GetSnapshotsRequest.SortBy sortBy, - SortOrder order, - Object fromSortValue - ) { + private static List allSnapshots(String[] snapshotNames, SnapshotSortKey sortBy, SortOrder order, Object fromSortValue) { return clusterAdmin().prepareGetSnapshots(matchAllPattern()) .setSnapshots(snapshotNames) .setSort(sortBy) @@ -660,12 +639,12 @@ public class GetSnapshotsIT extends AbstractSnapshotIntegTestCase { return clusterAdmin().prepareGetSnapshots(matchAllPattern()) .setSnapshots(matchAllPattern()) .setPolicies(policies) - .setSort(GetSnapshotsRequest.SortBy.NAME) + .setSort(SnapshotSortKey.NAME) .get() .getSnapshots(); } - private static void assertStablePagination(String[] repoNames, Collection allSnapshotNames, GetSnapshotsRequest.SortBy sort) { + private static void assertStablePagination(String[] repoNames, Collection allSnapshotNames, SnapshotSortKey sort) { final SortOrder order = randomFrom(SortOrder.values()); final List allSorted = allSnapshotsSorted(allSnapshotNames, repoNames, sort, order); @@ -700,7 +679,7 @@ public class GetSnapshotsIT extends AbstractSnapshotIntegTestCase { private static List allSnapshotsSorted( Collection allSnapshotNames, String[] repoNames, - GetSnapshotsRequest.SortBy sortBy, + SnapshotSortKey sortBy, SortOrder order, String... namePatterns ) { @@ -724,7 +703,7 @@ public class GetSnapshotsIT extends AbstractSnapshotIntegTestCase { private static GetSnapshotsResponse sortedWithLimit( String[] repoNames, - GetSnapshotsRequest.SortBy sortBy, + SnapshotSortKey sortBy, String after, int size, SortOrder order, @@ -738,13 +717,7 @@ public class GetSnapshotsIT extends AbstractSnapshotIntegTestCase { .get(); } - private static GetSnapshotsResponse sortedWithLimit( - String[] repoNames, - GetSnapshotsRequest.SortBy sortBy, - int offset, - int size, - SortOrder order - ) { + private static GetSnapshotsResponse sortedWithLimit(String[] repoNames, SnapshotSortKey sortBy, int offset, int size, SortOrder order) { return baseGetSnapshotsRequest(repoNames).setOffset(offset).setSort(sortBy).setSize(size).setOrder(order).get(); } diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsRequest.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsRequest.java index fda371f9364f..25373178e2b8 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsRequest.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsRequest.java @@ -61,7 +61,7 @@ public class GetSnapshotsRequest extends MasterNodeRequest @Nullable private String fromSortValue; - private SortBy sort = SortBy.START_TIME; + private SnapshotSortKey sort = SnapshotSortKey.START_TIME; private SortOrder order = SortOrder.ASC; @@ -106,7 +106,7 @@ public class GetSnapshotsRequest extends MasterNodeRequest ignoreUnavailable = in.readBoolean(); verbose = in.readBoolean(); after = in.readOptionalWriteable(After::new); - sort = in.readEnum(SortBy.class); + sort = in.readEnum(SnapshotSortKey.class); size = in.readVInt(); order = SortOrder.readFromStream(in); offset = in.readVInt(); @@ -146,7 +146,7 @@ public class GetSnapshotsRequest extends MasterNodeRequest validationException = addValidationError("size must be -1 or greater than 0", validationException); } if (verbose == false) { - if (sort != SortBy.START_TIME) { + if (sort != SnapshotSortKey.START_TIME) { validationException = addValidationError("can't use non-default sort with verbose=false", validationException); } if (size > 0) { @@ -287,7 +287,7 @@ public class GetSnapshotsRequest extends MasterNodeRequest return after; } - public SortBy sort() { + public SnapshotSortKey sort() { return sort; } @@ -306,7 +306,7 @@ public class GetSnapshotsRequest extends MasterNodeRequest return fromSortValue; } - public GetSnapshotsRequest sort(SortBy sort) { + public GetSnapshotsRequest sort(SnapshotSortKey sort) { this.sort = sort; return this; } @@ -350,40 +350,6 @@ public class GetSnapshotsRequest extends MasterNodeRequest return new CancellableTask(id, type, action, getDescription(), parentTaskId, headers); } - public enum SortBy { - START_TIME("start_time"), - NAME("name"), - DURATION("duration"), - INDICES("index_count"), - SHARDS("shard_count"), - FAILED_SHARDS("failed_shard_count"), - REPOSITORY("repository"); - - private final String param; - - SortBy(String param) { - this.param = param; - } - - @Override - public String toString() { - return param; - } - - public static SortBy of(String value) { - return switch (value) { - case "start_time" -> START_TIME; - case "name" -> NAME; - case "duration" -> DURATION; - case "index_count" -> INDICES; - case "shard_count" -> SHARDS; - case "failed_shard_count" -> FAILED_SHARDS; - case "repository" -> REPOSITORY; - default -> throw new IllegalArgumentException("unknown sort order [" + value + "]"); - }; - } - } - public static final class After implements Writeable { private final String value; @@ -405,7 +371,7 @@ public class GetSnapshotsRequest extends MasterNodeRequest } @Nullable - public static After from(@Nullable SnapshotInfo snapshotInfo, SortBy sortBy) { + public static After from(@Nullable SnapshotInfo snapshotInfo, SnapshotSortKey sortBy) { if (snapshotInfo == null) { return null; } diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsRequestBuilder.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsRequestBuilder.java index eadbaa8aa095..25e8a433bf24 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsRequestBuilder.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsRequestBuilder.java @@ -122,7 +122,7 @@ public class GetSnapshotsRequestBuilder extends MasterNodeOperationRequestBuilde return this; } - public GetSnapshotsRequestBuilder setSort(GetSnapshotsRequest.SortBy sort) { + public GetSnapshotsRequestBuilder setSort(SnapshotSortKey sort) { request.sort(sort); return this; } diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/SnapshotSortKey.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/SnapshotSortKey.java new file mode 100644 index 000000000000..599f41e8615d --- /dev/null +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/SnapshotSortKey.java @@ -0,0 +1,83 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +package org.elasticsearch.action.admin.cluster.snapshots.get; + +import org.elasticsearch.snapshots.SnapshotInfo; + +import java.util.Comparator; + +/** + * Sort key for snapshots e.g. returned from the get-snapshots API. All values break ties using {@link SnapshotInfo#snapshotId} (i.e. by + * name). + */ +public enum SnapshotSortKey { + /** + * Sort by snapshot start time. + */ + START_TIME("start_time", Comparator.comparingLong(SnapshotInfo::startTime)), + + /** + * Sort by snapshot name. + */ + NAME("name", Comparator.comparing(sni -> sni.snapshotId().getName())), + + /** + * Sort by snapshot duration (end time minus start time). + */ + DURATION("duration", Comparator.comparingLong(sni -> sni.endTime() - sni.startTime())), + /** + * Sort by number of indices in the snapshot. + */ + INDICES("index_count", Comparator.comparingInt(sni -> sni.indices().size())), + + /** + * Sort by number of shards in the snapshot. + */ + SHARDS("shard_count", Comparator.comparingInt(SnapshotInfo::totalShards)), + + /** + * Sort by number of failed shards in the snapshot. + */ + FAILED_SHARDS("failed_shard_count", Comparator.comparingInt(SnapshotInfo::failedShards)), + + /** + * Sort by repository name. + */ + REPOSITORY("repository", Comparator.comparing(SnapshotInfo::repository)); + + private final String name; + private final Comparator snapshotInfoComparator; + + SnapshotSortKey(String name, Comparator snapshotInfoComparator) { + this.name = name; + this.snapshotInfoComparator = snapshotInfoComparator.thenComparing(SnapshotInfo::snapshotId); + } + + @Override + public String toString() { + return name; + } + + public final Comparator getSnapshotInfoComparator() { + return snapshotInfoComparator; + } + + public static SnapshotSortKey of(String name) { + return switch (name) { + case "start_time" -> START_TIME; + case "name" -> NAME; + case "duration" -> DURATION; + case "index_count" -> INDICES; + case "shard_count" -> SHARDS; + case "failed_shard_count" -> FAILED_SHARDS; + case "repository" -> REPOSITORY; + default -> throw new IllegalArgumentException("unknown sort key [" + name + "]"); + }; + } +} diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/TransportGetSnapshotsAction.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/TransportGetSnapshotsAction.java index ce3446317400..c664e51a6845 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/TransportGetSnapshotsAction.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/TransportGetSnapshotsAction.java @@ -148,7 +148,7 @@ public class TransportGetSnapshotsAction extends TransportMasterNodeAction maybeFilterRepositories() { - if (sortBy != GetSnapshotsRequest.SortBy.REPOSITORY || fromSortValue == null) { + if (sortBy != SnapshotSortKey.REPOSITORY || fromSortValue == null) { return repositories; } final Predicate predicate = order == SortOrder.ASC @@ -486,27 +486,6 @@ public class TransportGetSnapshotsAction extends TransportMasterNodeAction BY_START_TIME = Comparator.comparingLong(SnapshotInfo::startTime) - .thenComparing(SnapshotInfo::snapshotId); - - private static final Comparator BY_DURATION = Comparator.comparingLong( - sni -> sni.endTime() - sni.startTime() - ).thenComparing(SnapshotInfo::snapshotId); - - private static final Comparator BY_INDICES_COUNT = Comparator.comparingInt(sni -> sni.indices().size()) - .thenComparing(SnapshotInfo::snapshotId); - - private static final Comparator BY_SHARDS_COUNT = Comparator.comparingInt(SnapshotInfo::totalShards) - .thenComparing(SnapshotInfo::snapshotId); - - private static final Comparator BY_FAILED_SHARDS_COUNT = Comparator.comparingInt(SnapshotInfo::failedShards) - .thenComparing(SnapshotInfo::snapshotId); - - private static final Comparator BY_NAME = Comparator.comparing(sni -> sni.snapshotId().getName()); - - private static final Comparator BY_REPOSITORY = Comparator.comparing(SnapshotInfo::repository) - .thenComparing(SnapshotInfo::snapshotId); - private SnapshotsInRepo sortSnapshotsWithNoOffsetOrLimit(List snapshotInfos) { return sortSnapshots(snapshotInfos.stream(), snapshotInfos.size(), 0, GetSnapshotsRequest.NO_LIMIT); } @@ -532,15 +511,7 @@ public class TransportGetSnapshotsAction extends TransportMasterNodeAction buildComparator() { - final Comparator comparator = switch (sortBy) { - case START_TIME -> BY_START_TIME; - case NAME -> BY_NAME; - case DURATION -> BY_DURATION; - case INDICES -> BY_INDICES_COUNT; - case SHARDS -> BY_SHARDS_COUNT; - case FAILED_SHARDS -> BY_FAILED_SHARDS_COUNT; - case REPOSITORY -> BY_REPOSITORY; - }; + final var comparator = sortBy.getSnapshotInfoComparator(); return order == SortOrder.DESC ? comparator.reversed() : comparator; } @@ -732,7 +703,7 @@ public class TransportGetSnapshotsAction extends TransportMasterNodeAction snapshotInfos, - @Nullable GetSnapshotsRequest.SortBy sort, - SortOrder sortOrder - ) { + public static void assertSnapshotListSorted(List snapshotInfos, @Nullable SnapshotSortKey sort, SortOrder sortOrder) { final BiConsumer assertion; if (sort == null) { assertion = (s1, s2) -> assertThat(s2, greaterThanOrEqualTo(s1));