Implement Sort By Repository Name in Get Snapshots API#77049
Implement Sort By Repository Name in Get Snapshots API#77049original-brownbear merged 1 commit intoelastic:masterfrom original-brownbear:sort-by-repository-name
Conversation
This one is the last sort column not yet implemented but used by Kibana.
|
Pinging @elastic/es-distributed (Team:Distributed) |
| public static final Version NUMERIC_PAGINATION_VERSION = Version.V_7_15_0; | ||
|
|
||
| private static final Version SORT_BY_SHARD_COUNTS_VERSION = Version.V_7_16_0; | ||
| private static final Version SORT_BY_SHARDS_OR_REPO_VERSION = Version.V_7_16_0; |
There was a problem hiding this comment.
Being a little lazy here :) <= we don't run BwC tests that would fail because of this and this is going to backport problem free.
arteam
left a comment
There was a problem hiding this comment.
LGTM, I left a couple of small comments
| out.writeOptionalWriteable(after); | ||
| if ((sort == SortBy.SHARDS || sort == SortBy.FAILED_SHARDS) && out.getVersion().before(SORT_BY_SHARD_COUNTS_VERSION)) { | ||
| throw new IllegalArgumentException("can't use sort by shard count with node version [" + out.getVersion() + "]"); | ||
| if ((sort == SortBy.SHARDS || sort == SortBy.FAILED_SHARDS || sort == SortBy.REPOSITORY) |
There was a problem hiding this comment.
I was wondering if it would make sense to extract all the 7.x incombatible parameters to a final set and then do a set.contains check instead of a combination of or disjunctions.
There was a problem hiding this comment.
I suppose we could but I think it's a little easier to follow this exceptional path by spelling it out directly. The performance here doesn't matter much with the request rates expected here so I think keeping it shorter is fine.
| ); | ||
| break; | ||
| case REPOSITORY: | ||
| isAfter = order == SortOrder.ASC |
There was a problem hiding this comment.
I would add parenthesis around order == SortOrder.ASC to make a clear distinction between the assignment and the ternary operator. Otherwise the reader needs to know the order priority of Java operators to know that isAfter = order doesn't get executed :)
There was a problem hiding this comment.
I would be OK to expect that of the reader, but if a parenthesis is added, it should cover the entire ternary expression.
| final List<String> snapshotNamesWithIndexA = createNSnapshots(repoNameA, randomIntBetween(3, 20)); | ||
| final List<String> snapshotNamesWithIndexB = createNSnapshots(repoNameB, randomIntBetween(3, 20)); | ||
|
|
||
| final Collection<String> allSnapshotNamesA = new HashSet<>(snapshotNamesWithIndexA); |
There was a problem hiding this comment.
Suggestion: What do you think about creating the set in one-line with the Streams API?
Stream.concat(snapshotNamesWithIndexA.stream(), snapshotNamesWithoutIndexA.stream()).collect(toSet())There was a problem hiding this comment.
Not the biggest fan to be honest but that's just personal preference :)
| ); | ||
| break; | ||
| case REPOSITORY: | ||
| isAfter = order == SortOrder.ASC |
There was a problem hiding this comment.
I would be OK to expect that of the reader, but if a parenthesis is added, it should cover the entire ternary expression.
|
Thanks Henning & Artem! |
This one is the last sort column not yet implemented but used by Kibana.