Add upgrade_only_ancient_segments option to upgrade API #10540
Conversation
mikemccand
added
>enhancement
v2.0.0-beta1
:Core/Infra/Core
| The `upgrade` API accepts the following request parameters: | ||
|
|
||
| [horizontal] | ||
| `upgrade_only_ancient_segments`:: If true, only very old segments (from a |
There was a problem hiding this comment.
Can upgrade be omitted here since its redundant with the upgrade api name?
There was a problem hiding this comment.
Also can we make a private boolean method shouldUpgrade for the hairy boolean logic. Github review tool just broke so i cant put this comment where it belongs.
| runUpgrade(httpClient(), indexToUpgrade, "only_ancient_segments", "true"); | ||
|
|
||
| // TODO: why do we never test upgrading ancient segments here...? | ||
| // E.g. 0.90.x (Lucene 3.x) --> 1.x (Lucene 4.x) ElasticsearchBackwardsCompatIntegrationTest.backwardsCompatibilityPath checks that |
There was a problem hiding this comment.
Hrm, 0.90.x was 4.x right? The backcompat tests can only run on the same major version because of wire format changes between 1.x and 2.0. I think the only way to test this is with the static bwc tests. There used to be an "UpgradeReallyOldIndexTest" or something like that. Perhaps we should have that to test the ancient vs non ancient behavior, and leave the static bwc tests just doing a normal upgrade?
There was a problem hiding this comment.
Hrm, 0.90.x was 4.x right?
Oh you're right!
OK I'll add a static index based on UpgradeReallyOldIndexTest on 1.x ..
|
I don't have any complains with this though if @rjernst is happy this LGTM |
|
OK I pushed more changes. I think this is ready. I added a new test case with a static back compat index containing a mix of ancient (Lucene 3.x) and merely old (Lucene 4.x) segments, and confirmed if I upgrade with only_ancient then the "merely old" segments remain. However, I saw this spooky-yet-apparently-harmless exception on upgrade: It's caused by a 0-byte translog file that 0.20.6 left behind even after I flushed the index. It doesn't cause the test to fail (ES really does just ignore it) but it still seems bad... though maybe it's just a known issue with 0.20.6? The 0 byte translog that 0.20.6 wrote somehow survived after upgrade to 0.90.6, adding new docs and flushing. |
| if 'node' in vars(): | ||
|
|
||
| if node is not None: | ||
| # This only happens if we've hit an exception: | ||
| logging.info('Shutting down node with pid %d', node.pid) |
There was a problem hiding this comment.
maybe move this to a simple helper like shutdown_node(node) since it is exactly the same as above?
|
LGTM |
Good idea, I just pushed that. |
… an old Lucene version are upgraded This option defaults to false, because it is also important to upgrade the "merely old" segments since many Lucene improvements happen within minor releases. But you can pass true to do the minimal work necessary to upgrade to the next major Elasticsearch release. The HTTP GET upgrade request now also breaks out how many bytes of ancient segments need upgrading. Closes #10213 Closes #10540
|
When I merged to master I dropped the static mixed ancient and "merely old" segments test because we cannot test that until we have a 2.0 release. |
clintongormley
added
:Upgrade API
and removed
:Core/Infra/Core
PR is based on 1.x.
The new option defaults to false, because it's still important to upgrade old-but-same-Lucene-major-version to get improvements.
If it's set to true, only segments whose major version is older than current major version will be upgraded.
I also fixed the REST upgrade GET API to separately return bytes of ancient segments.
Does anyone know why UpgradeTest doesn't allow testing upgrading ancient segments...? ElasticsearchBackwardsCompatibilityIntegrationTest.backwardsCompatibilityPath gets angry because e.g. 0.90.X is < Version.V_1_6_0.minimumCompatibilityVersion() ... but I thought users are allowed to do such an upgrade?
Closes #10213