Implement Exclude Patterns for Snapshot- and Repository Names in Get Snapshots API #77308

original-brownbear · 2021-09-06T12:26:03Z

It's in the title. Adds support for exclude patterns combined with wildcard requests
similar to what we support for index names.

…Snapshots API It's in the title. Adds support for exclude patterns combined with wildcard requests similar to what we support for index names.

elasticmachine · 2021-09-06T12:26:07Z

Pinging @elastic/es-distributed (Team:Distributed)

tlrx

LGTM - I left minor comments, feel free to address

docs/reference/snapshot-restore/apis/get-snapshot-api.asciidoc

server/src/internalClusterTest/java/org/elasticsearch/snapshots/GetSnapshotsIT.java

tlrx · 2021-09-07T07:43:05Z

server/src/internalClusterTest/java/org/elasticsearch/snapshots/GetSnapshotsIT.java

+            "-" + otherPrefixSnapshot1,
+            "-" + otherPrefixSnapshot2
+        );
+        assertThat(allInOtherWithoutOtherExplicit, is(allInOther));


Should we test the result when a snapshot is both included and excluded?

++ added explicit check for this case

tlrx · 2021-09-07T07:55:12Z

.../org/elasticsearch/action/admin/cluster/repositories/get/TransportGetRepositoriesAction.java

            for (String repositoryOrPattern : repoNames) {
-                if (Regex.isSimpleMatchPattern(repositoryOrPattern) == false) {
+                if (seenWildcard && repositoryOrPattern.length() > 1 && repositoryOrPattern.startsWith("-")) {


I suppose we could iterate 2 times over the repositories, the first time to match repositories against include patterns ad the second to filter out matching repos with exclude patterns? Anyway the current code works too.

Can we perhaps do this similar to the slm-change, where we first derive the include/exclude patterns and then run through the repos? And then extract the logic to a shared method to use such that we are sure that the include/exclude logic works the same.

I dried up and shortened the logic quite a bit now across repos and snapshots but I couldn't use the exact same code because of the special case handling for stuff like _current and 404 behavior. Still should be more efficient and easier to read now I hope.

arteam · 2021-09-07T09:49:14Z

server/src/internalClusterTest/java/org/elasticsearch/snapshots/GetSnapshotsIT.java

+        final List<String> namesRepo2 = createNSnapshots(repoName2, randomIntBetween(1, 5));
+        final List<String> namesOtherRepo = createNSnapshots(otherRepo, randomIntBetween(1, 5));
+
+        final Collection<String> allSnapshotNames = new HashSet<>(namesRepo1);


Consider concatenating repos with the Streams API:

Stream.concat(namesRepo1.stream(), namesRepo2.stream()).collect(Collectors.toSet())

.../org/elasticsearch/action/admin/cluster/repositories/get/TransportGetRepositoriesAction.java

henningandersen

Left a few comments to consider.

henningandersen · 2021-09-07T10:15:25Z

.../org/elasticsearch/action/admin/cluster/repositories/get/TransportGetRepositoriesAction.java

            for (String repositoryOrPattern : repoNames) {
-                if (Regex.isSimpleMatchPattern(repositoryOrPattern) == false) {
+                if (seenWildcard && repositoryOrPattern.length() > 1 && repositoryOrPattern.startsWith("-")) {


Can we perhaps do this similar to the slm-change, where we first derive the include/exclude patterns and then run through the repos? And then extract the logic to a shared method to use such that we are sure that the include/exclude logic works the same.

henningandersen · 2021-09-07T10:16:58Z

...n/java/org/elasticsearch/action/admin/cluster/snapshots/get/TransportGetSnapshotsAction.java

        if (isAllSnapshots(snapshots)) {
            toResolve.addAll(allSnapshotIds.values());
        } else {
            for (String snapshotOrPattern : snapshots) {
-                if (GetSnapshotsRequest.CURRENT_SNAPSHOT.equalsIgnoreCase(snapshotOrPattern)) {
+                if (seenWildcard && snapshotOrPattern.length() > 1 && snapshotOrPattern.startsWith("-")) {


similarly to the repo-name matching, can we derive the include/exclude pattern lists first and share most of the logic between the two (and slm-policy)?

++ makes sense, the logic isn't exactly the same because of the concrete snapshot name -> not found -> throw case but I'll dry it up. I'll wait for the other PR to land to continue here then :)

On second thought, no we can't use the same logic as there's a difference between how the resolution works for policies and repositories/snapshots. For repositories and snapshots, if we have a concrete/non-wildcard pattern in the mix, we throw a 404ish exception if we can't find the request repo/snapshot. We don't do any such thing for SLM policies which never cause exceptions to be thrown (which makes sense IMO). I'll try to simplify the logic a little regardless :)

I would think we could extract the include/exclude patterns in one method shared between multiple purposes? And then run through them afterwards handling the exceptions etc.

The thing is, we have different behavior between the repos API (where we loop over the patterns and then nest the repos under them) while we can do the more efficient way of going over all the snapshot names and then matching against all patterns in one go (because we order the repos according to the order of the patterns instead of by their name or so).
It doesn't really seem like there's a simple way of drying this up but then again note that we're going to move the snapshot name logic away from running on heap like this in a follow-up, so I'm not sure there's too much point in abstracting things away only for that abstraction to become redundant in a week?

server/src/internalClusterTest/java/org/elasticsearch/snapshots/GetSnapshotsIT.java

henningandersen · 2021-09-07T10:35:58Z

server/src/internalClusterTest/java/org/elasticsearch/snapshots/GetSnapshotsIT.java

+            patternOtherRepo,
+            GetSnapshotsRequest.SortBy.REPOSITORY,
+            order,
+            "-" + otherPrefixSnapshot1,


nit: it seems odd that for the repo pattern we pass in a comma-separated string, whereas for the names we pass in a list of name patterns. I would prefer to see this harmonized.

harmonized to using arrays/var-arg for both :)

henningandersen · 2021-09-07T10:39:02Z

server/src/internalClusterTest/java/org/elasticsearch/snapshots/GetSnapshotsIT.java

@@ -208,6 +209,67 @@ public void testPaginationRequiresVerboseListing() throws Exception {
        );
    }

+    public void testExcludePatterns() throws Exception {


Can we add a test validating that using -xyz finds such a snapshot (and repo) if no wildcard is specified first, i.e., at that we can still find snapshots/repos starting with -?

Good point! -> added a couple of tests around this scenario now.

…napshot-patterns

original-brownbear · 2021-09-08T10:42:34Z

Thanks Henning, all points addressed now (though I couldn't dry things up as requested I'm afraid).

original-brownbear · 2021-09-08T11:01:16Z

Jenkins run elasticsearch-ci/packaging-tests-windows-sample

original-brownbear · 2021-09-08T11:34:47Z

Jenkins run elasticsearch-ci/packaging-tests-windows-sample

original-brownbear · 2021-09-08T11:46:44Z

@elasticmachine update branch

henningandersen

A few smaller comments. Still hoping for a bit of code/sharing on include/exclude patterns 🙂

henningandersen · 2021-09-09T09:58:35Z

docs/reference/snapshot-restore/apis/get-snapshot-api.asciidoc

 +
 To get information about all snapshot repositories registered in the
 cluster, omit this parameter or use `*` or `_all`.

 `<snapshot>`::
 (Required, string)
-Comma-separated list of snapshot names to retrieve. Also accepts wildcards (`*`).
+Comma-separated list of snapshot names to retrieve. Also accepts wildcards (`*`) as well as combining wildcards and exclude patterns


Can we align the text between repo and name match? Otherwise I end up searching for the difference between the text to understand if there is some difference...
Perhaps just state

Wildcard (`*`) expressions are supported including combining wildcards with exclude patterns starting with `-`

like above?

Right, that was pointless, made it the same for both as suggested now.

henningandersen · 2021-09-09T10:09:14Z

...n/java/org/elasticsearch/action/admin/cluster/snapshots/get/TransportGetSnapshotsAction.java

        if (isAllSnapshots(snapshots)) {
            toResolve.addAll(allSnapshotIds.values());
        } else {
            for (String snapshotOrPattern : snapshots) {
-                if (GetSnapshotsRequest.CURRENT_SNAPSHOT.equalsIgnoreCase(snapshotOrPattern)) {
+                if (seenWildcard && snapshotOrPattern.length() > 1 && snapshotOrPattern.startsWith("-")) {


I would think we could extract the include/exclude patterns in one method shared between multiple purposes? And then run through them afterwards handling the exceptions etc.

henningandersen · 2021-09-09T10:22:29Z

.../org/elasticsearch/action/admin/cluster/repositories/get/TransportGetRepositoriesAction.java

-                if (Regex.isSimpleMatchPattern(repositoryOrPattern) == false) {
-                    repositoriesToGet.add(repositoryOrPattern);
+        }
+        final List<String> includePatterns = new ArrayList<>();


I think the new exclude functionality also affects the list repositories API, we should also update the documentation for that API then.

henningandersen · 2021-09-09T10:26:02Z

server/src/internalClusterTest/java/org/elasticsearch/snapshots/GetSnapshotsIT.java

+                )
+            )
+        );
+    }


Can we also verify that non-existing*,-weird-snapshot-1 does not return -weird-snapshot-1? And that *,--weird-snapshot-1 also does not return it? And that -* returns all - prefixed snapshots.

++ added those cases :)

…napshot-patterns

…ns' into support-negative-snapshot-patterns

original-brownbear · 2021-09-09T15:24:42Z

Thanks @henningandersen! addresed all points but sharing the pattern resolution code, to me at least it seems with the way we do the ordering and not-ordering when resolving things, it's going to be pain to share code at this point in time I think, but lets see what you think :)

…napshot-patterns

original-brownbear · 2021-09-10T10:16:07Z

Alright @henningandersen this should be good for another round. Implemented _current as wildcard as well as fixed the pattern,name,excludePatternForName case we discussed last night and added tests for both :)

henningandersen

LGTM.

henningandersen · 2021-09-09T16:12:18Z

docs/reference/snapshot-restore/apis/get-snapshot-api.asciidoc

 +
 To get information about all snapshot repositories registered in the
 cluster, omit this parameter or use `*` or `_all`.

 `<snapshot>`::
 (Required, string)
-Comma-separated list of snapshot names to retrieve. Also accepts wildcards (`*`).


I think we still want to include this bit "Comma-separated list of snapshot names to retrieve. "?

…napshot-patterns

original-brownbear · 2021-09-10T13:00:21Z

Thanks everyone!

…Snapshots API (elastic#77308) It's in the title. Adds support for exclude patterns combined with wildcard requests similar to what we support for index names.

…Snapshots API (#77308) (#79315) It's in the title. Adds support for exclude patterns combined with wildcard requests similar to what we support for index names.

Implement Exclude Patterns for Snapshot- and Repository Names in Get …

654eeb1

…Snapshots API It's in the title. Adds support for exclude patterns combined with wildcard requests similar to what we support for index names.

original-brownbear added >enhancement :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v8.0.0 v7.16.0 labels Sep 6, 2021

elasticmachine added the Team:Distributed Meta label for distributed team (obsolete) label Sep 6, 2021

original-brownbear mentioned this pull request Sep 6, 2021

Improve Snapshot Repository Scalability #74350

Closed

16 tasks

original-brownbear requested review from henningandersen and tlrx September 6, 2021 13:54

tlrx approved these changes Sep 7, 2021

View reviewed changes

arteam self-requested a review September 7, 2021 08:30

arteam reviewed Sep 7, 2021

View reviewed changes

henningandersen reviewed Sep 7, 2021

View reviewed changes

original-brownbear added 9 commits September 7, 2021 13:52

Merge remote-tracking branch 'elastic/master' into support-negative-s…

c3b989c

…napshot-patterns

Merge remote-tracking branch 'elastic/master' into support-negative-s…

b0bc904

…napshot-patterns

Merge remote-tracking branch 'elastic/master' into support-negative-s…

c0af03c

…napshot-patterns

Merge remote-tracking branch 'elastic/master' into support-negative-s…

38e60c2

…napshot-patterns

Merge remote-tracking branch 'elastic/master' into support-negative-s…

564eef8

…napshot-patterns

make logic for resolving things nicer

6e4f0d9

align pattern usage

081633a

test with dash

74e76f6

CR comments

de70f55

original-brownbear requested a review from henningandersen September 8, 2021 10:41

Merge branch 'master' into support-negative-snapshot-patterns

61b5731

henningandersen reviewed Sep 9, 2021

View reviewed changes

original-brownbear added 5 commits September 9, 2021 12:36

Merge remote-tracking branch 'elastic/master' into support-negative-s…

7e05c41

…napshot-patterns

Merge remote-tracking branch 'elastic/master' into support-negative-s…

edf540f

…napshot-patterns

tests

fc296ed

Merge remote-tracking branch 'elastic/master' into support-negative-s…

afa4afd

…napshot-patterns

Merge remote-tracking branch 'origin/support-negative-snapshot-patter…

6f22652

…ns' into support-negative-snapshot-patterns

original-brownbear requested a review from henningandersen September 9, 2021 15:23

original-brownbear added 3 commits September 10, 2021 10:39

Merge remote-tracking branch 'elastic/master' into support-negative-s…

60c9b81

…napshot-patterns

desired concrete name + wildcard behavior

bcecd6f

_current is a wildcard pattern now

ffe7484

henningandersen approved these changes Sep 10, 2021

View reviewed changes

original-brownbear added 2 commits September 10, 2021 13:51

Merge remote-tracking branch 'elastic/master' into support-negative-s…

2110176

…napshot-patterns

missing sentence

e7aa966

original-brownbear merged commit f1a4551 into elastic:master Sep 10, 2021

original-brownbear deleted the support-negative-snapshot-patterns branch September 10, 2021 13:00

original-brownbear added the backport pending label Sep 10, 2021

jakelandis added v8.0.0-alpha2 and removed v8.0.0 labels Sep 15, 2021

original-brownbear mentioned this pull request Oct 17, 2021

Implement Exclude Patterns for Snapshot- and Repository Names in Get Snapshots API (#77308) #79315

Merged

original-brownbear removed the backport pending label Oct 17, 2021

original-brownbear restored the support-negative-snapshot-patterns branch April 18, 2023 20:38

Implement Exclude Patterns for Snapshot- and Repository Names in Get Snapshots API #77308

Implement Exclude Patterns for Snapshot- and Repository Names in Get Snapshots API #77308

Conversation

original-brownbear commented Sep 6, 2021

elasticmachine commented Sep 6, 2021

tlrx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henningandersen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

original-brownbear commented Sep 8, 2021

original-brownbear commented Sep 8, 2021

original-brownbear commented Sep 8, 2021

original-brownbear commented Sep 8, 2021

henningandersen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henningandersen Sep 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

original-brownbear commented Sep 9, 2021

original-brownbear commented Sep 10, 2021

henningandersen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

original-brownbear commented Sep 10, 2021

henningandersen Sep 9, 2021 •

edited

Loading