-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Migrate getProject().index calls to lookup index #124178
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
Migrate getProject().index calls to lookup index #124178
Conversation
This migrates a number of calls of the form
metadata.getProject().getIndexSafe(index)
or
metadata.getProject().index(index)
to
metadata.indexMetadata(index)
or
metadata.findIndex(index)
|
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
nielsbauman
left a comment
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.
Left one optional comment, other than that LGTM.
| for (ShardRouting shardRouting : routingNode) { | ||
| var indexMetadata = metadata.getProject().index(shardRouting.index()); | ||
| var indexMetadata = metadata.indexMetadata(shardRouting.index()); | ||
| var shardSize = clusterInfo.getShardSize(shardRouting, 0L); | ||
| assert indexMetadata != null; |
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.
Nit: in this case, I'd argue the assertion loses its value because a non-existent index (or project) would have already thrown an exception.
ywangd
left a comment
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
Not for this PR: I wonder whether we should make these methods more consistent:
- ProjectMetadata#index
- ProjectMetadata#getIndexSafe
- Metadata#findIndex
- Metadata#indexMetadata
The issues I can see are:
- Inconsistent naming style, e.g. getter vs builder pattern
- Except 2, they aer not clear which ones are throwing on null value
- 1 returns null while 3 returns Optional
- 4 spells out the full
indexMetadatadespite the other 3 are also about index metadata
The result is that I have to check up their usages each time I need to use them or review a PR containing their usages. I think it would be great if they are renamed more consistently and more obviously.
It's come up before, and I think it makes sense. I'm not planning to tackle it, but it would be good for someone else to do so. |
This migrates a number of calls of the form
metadata.getProject().getIndexSafe(index)
or
metadata.getProject().index(index)
to
metadata.indexMetadata(index)
or
metadata.findIndex(index)
This migrates a number of calls of the form
metadata.getProject().getIndexSafe(index)
or
metadata.getProject().index(index)
to
metadata.indexMetadata(index)
or
metadata.findIndex(index)
This migrates a number of calls of the form
or
to
or