-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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.
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
Add Sort By Shard Count and Failed Shard Count to Get Snapshots API #77011
Add Sort By Shard Count and Failed Shard Count to Get Snapshots API #77011
Conversation
It's in the title. As requested by the Kibana team, adding these two additional sort columns.
Pinging @elastic/es-distributed (Team:Distributed) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I've left a couple of docs/naming comments.
Sort snapshots by the number of shards they contain and break ties by snapshot name. | ||
|
||
`failed_shards_count`:: | ||
Sort snapshots by the number of shards that they failed to snapshot break ties by snapshot name. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that they failed to snapshot
I believe they
is redundant here: the number of shards that failed to snapshot
sounds more natural to me.
failed to snapshot break ties by snapshot name
I guess and
is missed here: failed to snapshot and break ties by snapshot name
@@ -120,6 +120,12 @@ Allows setting a sort order for the result. Defaults to `start_time`, i.e. sorti | |||
|
|||
`index_count`:: | |||
Sort snapshots by the number of indices they contain and break ties by snapshot name. | |||
|
|||
`shard_count`:: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General comment: I'm not sure if the parameter names are set in stone, but I think it's inconsistent that shard_count
is singular and failed_shards_count
is plural. E.g. we refer to shard_count
in the codebase as SHARDS
and SHARDS_COUNT
, but in the external API it's shard_count
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++ that makes good sense, I made the external parameter failed_shard_count
now to make it consistent. I think the enum/internal names are ok as is and we wouldn't really want to change them at this point for BwC reasons (at least technically the transport client would have its API broken otherwise).
Thanks Henning + Artem! |
…lastic#77011) It's in the title. As requested by the Kibana team, adding these two additional sort columns. relates elastic#74350
It's in the title. As requested by the Kibana team, adding these two additional sort columns.