Make index uuid available in Index, ShardRouting & ShardId #16217
Conversation
bleskes
changed the title
|
|
||
| private Index() { | ||
|
|
||
| } | ||
|
|
||
| public Index(String name) { | ||
| public Index(String name, String uuid) { |
There was a problem hiding this comment.
can we please get rid of the interning? I think we should do this in a sep PR before this get's merged
There was a problem hiding this comment.
sure - didn't want to make things controversial. I'll make a PR to remove the name.intern()
|
I love this - infact I started the same effort last week but didn't get too far time wise. I am +100 on this. I really think we have to try to get rid of the |
|
@s1monw I rebased on master and addressed your comments. All test pass now. Can you take another look? |
| } | ||
| } else { | ||
| minimumCompatibleLuceneVersion = null; | ||
| } | ||
|
|
||
| return new IndexMetaData(index, version, state, numberOfShards, numberOfReplicas, tmpSettings, mappings.build(), | ||
| final String uuid = settings.get(SETTING_INDEX_UUID, INDEX_UUID_NA_VALUE); |
There was a problem hiding this comment.
I am not sure but shouldn't this one be a generated one if it's not in the settings?
There was a problem hiding this comment.
At least at the moment the uuid generation is done on MetaDataCreateIndexService#createIndex - here we just create a IndexMetaData object, not knowing if it's new or old. The way I did it mimics what the old code did (by return this from the settings directly). I want to keep the semantic change as small as possible.
|
@bleskes LGTM I left minor comments in the code, can we maybe also add an assertion that the uuid is a real UUID when we create IndexService? that would be awesome. No need for another review!!! thanks so much for doing this |
| @@ -76,7 +76,7 @@ public boolean equals(Object o) { | |||
| if (this == o) return true; | |||
| if (o == null) return false; | |||
| ShardId shardId1 = (ShardId) o; | |||
| return shardId == shardId1.shardId && index.name().equals(shardId1.index.name()); | |||
| return shardId == shardId1.shardId && index.getName().equals(shardId1.index.getName()); | |||
There was a problem hiding this comment.
I think this should be:
return shardId == shardId1.shardId && index.equals(shardId1.index);
In the early days Elasticsearch used to use the index name as the index identity. Around 1.0.0 we introduced a unique index uuid which is stored in the index setting. Since then we used that uuid in a few places but it is by far not the main identifier when working with indices, partially because it's not always readily available in all places.
This PR start to make a move in the direction of using uuids instead of name by making sure that the uuid is available on the Index class (currently just a wrapper around the name) and as such also available via ShardRouting and ShardId.
Note that this is by no means an attempt to do the right thing with the uuid in all places. In almost all places it falls back to the name based comparison that was done before. It is meant as a first step towards slowly improving the situation.
PS. This came from starting to think about implementing storing indices on disk using folders
NAME_UUIDpattern (instead of just name as we do today) and thus being able to avoid collision between indices that are named the same (but deleted and recreated). The hope is that this will als remove the need for the in memory shard locks we have now. The problem was that I didn't have easy access to the uuid in all places.While the tests are not all passing ( 99% of them are). I think this is ready to get initial feedback.