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
Check for incompatible mappings while upgrading old indices #12406
Check for incompatible mappings while upgrading old indices #12406
Conversation
@imotov correct me if I'm wrong, but since we now check the mapping in the constructor of GatewayMetaState, if it runs into a mapping that's incompatible with 2.0 it will throw an exception and prevent the node from starting... |
@bleskes you are right and this is the behavior described in the commit comment. It is consistent with a node behavior in case of other detected incompatibilities. We had a discussion about this and came to the conclusion that this is the best user experience we can provide. A user trying to upgrade a cluster with incompatible indices will not be able to start a node and since we haven't updated any metadata or data files at this point yet, they still will be able to downgrade to the previous version and fix the issue before trying to upgrade again. An alternative would be to start the cluster with potentially all indices closed and no way back. |
@imotov i like this approach |
d6cd326
to
3fb3401
Compare
@@ -32,7 +33,9 @@ | |||
*/ | |||
class ShardUpgradeResponse extends BroadcastShardResponse { | |||
|
|||
private org.apache.lucene.util.Version version; | |||
private org.apache.lucene.util.Version luceneVersion; |
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.
If my understanding is correct, this is the version of the "oldest" segment in the index? If yes, can you document it: I had to think about it for a bit since the elasticsearch version below also points to a lucene version which is the version which has been used to write the index initially.
@imotov I like the behaviour. I guess something that I'm missing from the PR is an upgrade test where there are two types that have a common field which has the same mappings, to make sure we don't reject that case? |
@jpountz because I couldn't create conflicting mapping in 2.0, I had to generate an index with a conflicting mapping using this script and then test it as part UpgradeReallyOldIndexTest. |
// oldest version that we need to support | ||
if (shardUpgradeResponse.oldestLuceneSegment().onOrAfter(versionTuple.v2()) == false) { | ||
luceneVersion = shardUpgradeResponse.oldestLuceneSegment(); | ||
} |
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.
the comment says it looks for the oldest version but the code looks for the newest version?
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.
sorry I missed the ==false
@jpountz I pushed an update. |
|
||
@Override | ||
public void close() { | ||
fakeAnalyzer.close(); |
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.
I don't think we need to close the fake analyzer since it doesn't hold resources?
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.
The parent class has a non-empty close method, so while you are right - there is nothing really happens there, I don't think it causes any harm to close it and it will make reasoning about the code easier.
This looks good to me in general but I would appreciate if someone else could give it a look to make sure I didn't overlook anything. |
/** | ||
* Checks the mappings of the mappings for compatibility with the current version | ||
*/ | ||
private IndexMetaData upgradeMappings(IndexMetaData indexMetaData) { |
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.
The upgradeMappings()
doesn't seem to modify the provided indexMetaData
variable, so I think this method should have void
as return type?
} | ||
|
||
public void testUpgradeConflictingMapping() throws Exception { | ||
String indexName = "index-1.7.0"; |
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.
Can we name this index differently to indicate it is an error case? Perhaps index-conflicting-mappings-1.7.0?
This looks good. I left a handful of minor comments/questions. |
@rjernst I pushed an update |
LGTM, just one minor comment about naming. |
Conflicting mappings that were allowed before v2.0 can cause runaway shard failures on upgrade. This commit adds a check that prevents a cluster from starting if it contains such indices as well as restoring such indices from a snapshot into already running cluster. Closes elastic#11857
2faa591
to
f71c9a2
Compare
Conflicting mappings that were allowed before v2.0 can cause runaway shard failures on upgrade. This commit adds a check that prevents a cluster from starting if it contains such indices as well as restoring such indices from a snapshot into already running cluster.
Closes #11857