Use the smallest version rather than the default version #11475
Conversation
| @@ -59,5 +62,6 @@ public void testUpgradeMixed_0_20_6_and_0_90_6() throws Exception { | |||
|
|
|||
| // We succeeded in upgrading only the ancient segments but leaving the "merely old" ones untouched: | |||
| assertTrue(UpgradeTest.hasOldButNotAncientSegments(client(), indexName)); | |||
| assertEquals(Version.CURRENT.luceneVersion.toString(), client().admin().indices().prepareGetSettings(indexName).get().getSetting(indexName, IndexMetaData.SETTING_VERSION_MINIMUM_COMPATIBLE)); | |||
| } | |||
There was a problem hiding this comment.
I'm confused about this assert. especially given the comments above it. If this minimum version is a lucene version, should it not be the oldest lucene version? In this case the ancient segments will be upgraded, but the "merely old" ones will still be there, so I can't see it being CURRENT.
|
I added some questions. One bigger question is: I don't like the hoops we are jumping through here. I also don't like the hoops lucene jumps through just to deliver the correct exception. Maybe lucene should store the minimum version inside the segments_N when we commit? If its an empty commit, its still populated with that version. This might simplify lucene itself on reading commits and make stuff like this easier for everyone. |
|
@rmuir I mixed something up you are right the test assertion didn't make sense |
|
+1 |
|
Also +1 to storing min version in segments_N @rmuir |
| luceneVersion = segment.getVersion(); | ||
| } | ||
| } | ||
| return luceneVersion; | ||
| return luceneVersion == null ? Version.indexCreated(indexSettings).luceneVersion : luceneVersion; | ||
| } |
There was a problem hiding this comment.
OK but this indexCreated is an immutable thing in ES right? So this means a user can never upgrade an empty index?
There was a problem hiding this comment.
if there are no docs in the index it won't do anything here yes! in this case we should make this method more low level - I will fix that too
|
+1 to have Lucene track this in segments_N |
|
I added more are asserts, I think the empty index is a different issue and should be solved elsewhere it's pretty tricky to get the version the segmetns_N file was written. I think we are good to go |
|
If this is an empty index, maybe we can just return |
the problem is that if we try to open an empty index with a version taht doesn't support this segments_N format we get an exception we can't recover from so somehow we need to be able to upgrade it |
|
+1 Lets spin off the empty index separately into another PR? We can leave a TODO for that. I still feel strongly it should be fixed, but this moves things forward. |
The minimum version comparison was always using the default version sicne the comparison was flipped. Closes elastic#11474
|
@s1monw we flush before running optimize and it overwrites the segments_N file even if there are no segments. So, shouldn't this make such index upgraded? |
The minimum version comparison was always using the default version
sicne the comparison was flipped.
Closes #11474