Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Standardize snapshot indices parsing so that combinations of included and excluded indices are treated the same regardless of the order they are listed in #5626

Merged
merged 14 commits into from
Jan 3, 2023
Merged

Standardize snapshot indices parsing so that combinations of included and excluded indices are treated the same regardless of the order they are listed in #5626

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
7a127d9
Added test cases
stephen-crawford Dec 22, 2022
827870c
reset snapshot utils file
stephen-crawford Dec 23, 2022
b1fa1fa
Merge branch 'opensearch-project:main' into restore-snapshot
stephen-crawford Dec 23, 2022
6a63feb
Add comparator
stephen-crawford Dec 23, 2022
a9a9c14
Add comparator and sorting method
stephen-crawford Dec 23, 2022
36deec5
Add changelog
stephen-crawford Dec 23, 2022
019a88f
fix import resolving
stephen-crawford Dec 23, 2022
2a43f9d
Spotless apply
stephen-crawford Dec 23, 2022
9a2ee3f
Remove length ==1
stephen-crawford Dec 23, 2022
5862e07
Add a double negated parsing test
stephen-crawford Dec 27, 2022
6beb70e
Standardize snapshot indices parsing so that combinations of included…
stephen-crawford Dec 30, 2022
39c7679
Merge branch 'main' into restore-snapshot
stephen-crawford Dec 30, 2022
4ea9c63
Spotless Apply
stephen-crawford Dec 30, 2022
c7d07bd
Fix index exclusion behavior in snapshot restore and clone APIs
stephen-crawford Jan 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
- Add CI bundle pattern to distribution download ([#5348](https://github.com/opensearch-project/OpenSearch/pull/5348))
- Add support for ppc64le architecture ([#5459](https://github.com/opensearch-project/OpenSearch/pull/5459))


### Dependencies
- Bumps `log4j-core` from 2.18.0 to 2.19.0
- Bumps `reactor-netty-http` from 1.0.18 to 1.0.23
Expand Down Expand Up @@ -65,6 +64,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
- Changed http code on create index API with bad input raising NotXContentException from 500 to 400 ([#4773](https://github.com/opensearch-project/OpenSearch/pull/4773))
- Change http code for DecommissioningFailedException from 500 to 400 ([#5283](https://github.com/opensearch-project/OpenSearch/pull/5283))
- Pre conditions check before updating weighted routing metadata ([#4955](https://github.com/opensearch-project/OpenSearch/pull/4955))
- Correct index parsing logic for SnapshotUtils ([5626]https://github.com/opensearch-project/OpenSearch/pull/5626)
stephen-crawford marked this conversation as resolved.
Show resolved Hide resolved

### Deprecated

Expand Down
16 changes: 15 additions & 1 deletion server/src/main/java/org/opensearch/snapshots/SnapshotUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,20 @@ public static List<String> filterIndices(List<String> availableIndices, String[]
if (IndexNameExpressionResolver.isAllIndices(Arrays.asList(selectedIndices))) {
return availableIndices;
}

selectedIndices = Arrays.stream(selectedIndices).filter(s -> !s.isEmpty()).toArray(a -> new String[a]); // Remove all empty strings
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really want to rewrite the logic in this method because it is very difficult to follow, but I'm going to resist that impulse for now. I am a bit concerned about possible implications of removing empty strings with the change here. I'm inclined to not sort the list, and instead just move the exclusions to the end. What do you think? That can be done with something like:

// Move the exclusions to end of list to ensure they are processed
// after explicitly selected indices are chosen.
final List<String> excludesAtEndSelectedIndices = Stream.concat(
    Arrays.stream(selectedIndices)
        .filter(s -> s.isEmpty() || s.charAt(0) != '-'),
    Arrays.stream(selectedIndices)
        .filter(s -> !s.isEmpty() && s.charAt(0) == '-'))
    .collect(Collectors.toUnmodifiableList());

Then just tweak the logic below to use this list instead of the selectedIndices array.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you think that this is a better option that seems fine. It should save a bit of time on larger indices lists (though the indices list probably won't be large enough that this is not negligible). I can swap this over :)

As far as the code logic, I only just started looking at this class and its methods but if you feel there is a way it can be improved that you would like to be done just let me know and I will rewrite it. I have just been trying to make the targeted changes with as little modification as possible so as to be less likely to break anything but I agree that the code could be a bit easier to follow.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have just been trying to make the targeted changes with as little modification as possible

This is definitely the right instinct and why we probably shouldn't completely rewrite this :)

Arrays.sort(selectedIndices, (o1, o2) -> { // Make '-' lower priority then everything

char o1FirstChar = o1.charAt(0);
char o2FirstChar = o2.charAt(0);
if (o1FirstChar == '-' && o2FirstChar != '-') {
return 1;
} else if (o1FirstChar != '-' && o2FirstChar == '-') {
return -1;
}
return o1.compareTo(o2);
});

Set<String> result = null;
for (int i = 0; i < selectedIndices.length; i++) {
String indexOrPattern = selectedIndices[i];
Expand All @@ -89,7 +103,7 @@ public static List<String> filterIndices(List<String> availableIndices, String[]
result = new HashSet<>();
}
} else if (indexOrPattern.charAt(0) == '-') {
// if its the first, fill it with all the indices...
// if its the first index pattern, fill it with all the indices...
stephen-crawford marked this conversation as resolved.
Show resolved Hide resolved
if (i == 0) {
result = new HashSet<>(availableIndices);
}
Expand Down
2 changes: 2 additions & 0 deletions server/src/test/java/org/opensearch/snapshots/SnapshotUtilsTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,8 @@ public void testIndexNameFiltering() {
assertIndexNameFiltering(new String[] { "foo", "bar", "baz" }, new String[] { "-ba*" }, new String[] { "foo" });
assertIndexNameFiltering(new String[] { "foo", "bar", "baz" }, new String[] { "+ba*" }, new String[] { "bar", "baz" });
assertIndexNameFiltering(new String[] { "foo", "bar", "baz" }, new String[] { "+bar", "+foo" }, new String[] { "bar", "foo" });
assertIndexNameFiltering(new String[] { "foo", "bar", "baz" }, new String[] { "-bar", "b*" }, new String[] { "baz" });
assertIndexNameFiltering(new String[] { "foo", "bar", "baz" }, new String[] { "b*", "-bar" }, new String[] { "baz" });
stephen-crawford marked this conversation as resolved.
Show resolved Hide resolved
assertIndexNameFiltering(
new String[] { "foo", "bar", "baz" },
new String[] { "zzz", "bar" },
Expand Down