Add assertBusy for testMappingVersionAfterDynamicMappingUpdate#38579
Closed
dakrone wants to merge 1 commit intoelastic:6.7from
Closed
Add assertBusy for testMappingVersionAfterDynamicMappingUpdate#38579dakrone wants to merge 1 commit intoelastic:6.7from
dakrone wants to merge 1 commit intoelastic:6.7from
Conversation
This assertBusy is necessary. When a mapping update is needed as a document is indexed, the document is tried, rejected (due to mapping conflict), then a mapping update sent off, the document is then *immediately* retried to see if the mapping change has occurred quickly enough, and if it has, indexing does not wait for the next cluster state to occur before moving ahead. In very rare cases this immediate retry succeeds, which causes the indexing request to complete (because it was successful) but the new cluster state to not be propagated entirely yet. In that case, we need to wait because the mapping version will eventually be updated, it just hasn't been updated *yet*. The first mapping-update-necessary check: https://github.com/elastic/elasticsearch/blob/622a7f1e207a552af56fec993045286abc3839e9/server/src/main/java/org/elasticsearch/action/bulk/TransportShardBulkAction.java#L487 Followed immediately by the mapping update: https://github.com/elastic/elasticsearch/blob/622a7f1e207a552af56fec993045286abc3839e9/server/src/main/java/org/elasticsearch/action/bulk/TransportShardBulkAction.java#L490 And then the *immediate* retry: https://github.com/elastic/elasticsearch/blob/622a7f1e207a552af56fec993045286abc3839e9/server/src/main/java/org/elasticsearch/action/bulk/TransportShardBulkAction.java#L499 In the event the immediate retry fails (99.9999% of the time), the context is marked as needing to wait for a new cluster state before proceeding: https://github.com/elastic/elasticsearch/blob/622a7f1e207a552af56fec993045286abc3839e9/server/src/main/java/org/elasticsearch/action/bulk/TransportShardBulkAction.java#L501-L504 In the 0.0001% case, the immediate retry succeeds, causing the test to fail. I was able to reproduce this bug about once every 10,000 tests. With the awaitsFix I ran this 100,000 times with no failures. Resolves elastic#38428
Collaborator
|
Pinging @elastic/es-distributed |
jakelandis
reviewed
Feb 11, 2019
| final long previousVersion = clusterService.state().metaData().index("test").getMappingVersion(); | ||
| client().prepareIndex("test", "type", "1").setSource("field", "text").get(); | ||
| assertThat(clusterService.state().metaData().index("test").getMappingVersion(), equalTo(1 + previousVersion)); | ||
| // This assertBusy is necessary. When a mapping update is needed as a document is indexed, |
Contributor
There was a problem hiding this comment.
Thanks for the detailed message, TIL. However, I think the commit message is sufficient for the detailed explanation. Perhaps a shorter version like:
//ensure that cluster state has been updated
Also, nitpick: s/mapping conflict/required mapping update
jakelandis
approved these changes
Feb 11, 2019
Contributor
jakelandis
left a comment
There was a problem hiding this comment.
LGTM with or without the detailed message. I have preference to shorten or remove it, but it is your call.
dakrone
added a commit
to dakrone/elasticsearch
that referenced
this pull request
Feb 13, 2019
Prior to this commit, when an indexing operation resulted in an `Engine.Result.Type.MAPPING_UPDATE_REQUIRED`, TransportShardBulkAction immediately retries the indexing operation to see if it succeeds. In the event that it succeeds the context does not wait until the mapping update has propagated through the cluster state before finishing the indexing. In some of our tests we rely on mappings being available as soon as they've been introduced in a document that indexed correctly. By removing the immediate retry we always wait for this to be the case. Resolves elastic#38428 Supercedes elastic#38579 Relates to elastic#38711
Member
Author
|
Closing this in favor of #38873 |
dakrone
added a commit
that referenced
this pull request
Feb 14, 2019
Prior to this commit, when an indexing operation resulted in an `Engine.Result.Type.MAPPING_UPDATE_REQUIRED`, TransportShardBulkAction immediately retries the indexing operation to see if it succeeds. In the event that it succeeds the context does not wait until the mapping update has propagated through the cluster state before finishing the indexing. In some of our tests we rely on mappings being available as soon as they've been introduced in a document that indexed correctly. By removing the immediate retry we always wait for this to be the case. Resolves #38428 Supercedes #38579 Relates to #38711
dakrone
added a commit
that referenced
this pull request
Feb 14, 2019
Prior to this commit, when an indexing operation resulted in an `Engine.Result.Type.MAPPING_UPDATE_REQUIRED`, TransportShardBulkAction immediately retries the indexing operation to see if it succeeds. In the event that it succeeds the context does not wait until the mapping update has propagated through the cluster state before finishing the indexing. In some of our tests we rely on mappings being available as soon as they've been introduced in a document that indexed correctly. By removing the immediate retry we always wait for this to be the case. Resolves #38428 Supercedes #38579 Relates to #38711
dakrone
added a commit
that referenced
this pull request
Feb 14, 2019
Prior to this commit, when an indexing operation resulted in an `Engine.Result.Type.MAPPING_UPDATE_REQUIRED`, TransportShardBulkAction immediately retries the indexing operation to see if it succeeds. In the event that it succeeds the context does not wait until the mapping update has propagated through the cluster state before finishing the indexing. In some of our tests we rely on mappings being available as soon as they've been introduced in a document that indexed correctly. By removing the immediate retry we always wait for this to be the case. Resolves #38428 Supercedes #38579 Relates to #38711
dakrone
added a commit
that referenced
this pull request
Feb 14, 2019
Prior to this commit, when an indexing operation resulted in an `Engine.Result.Type.MAPPING_UPDATE_REQUIRED`, TransportShardBulkAction immediately retries the indexing operation to see if it succeeds. In the event that it succeeds the context does not wait until the mapping update has propagated through the cluster state before finishing the indexing. In some of our tests we rely on mappings being available as soon as they've been introduced in a document that indexed correctly. By removing the immediate retry we always wait for this to be the case. Resolves #38428 Supercedes #38579 Relates to #38711
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This assertBusy is necessary. When a mapping update is needed as a document is
indexed, the document is tried, rejected (due to mapping conflict), then a
mapping update sent off, the document is then immediately retried to see if
the mapping change has occurred quickly enough, and if it has, indexing does not
wait for the next cluster state to occur before moving ahead. In very rare cases
this immediate retry succeeds, which causes the indexing request to
complete (because it was successful) but the new cluster state to not be
propagated entirely yet. In that case, we need to wait because the mapping
version will eventually be updated, it just hasn't been updated yet.
The first mapping-update-necessary check:
elasticsearch/server/src/main/java/org/elasticsearch/action/bulk/TransportShardBulkAction.java
Line 487 in 622a7f1
Followed immediately by the mapping update:
elasticsearch/server/src/main/java/org/elasticsearch/action/bulk/TransportShardBulkAction.java
Line 490 in 622a7f1
And then the immediate retry:
elasticsearch/server/src/main/java/org/elasticsearch/action/bulk/TransportShardBulkAction.java
Line 499 in 622a7f1
In the event the immediate retry fails (99.9999% of the time), the context is
marked as needing to wait for a new cluster state before proceeding:
elasticsearch/server/src/main/java/org/elasticsearch/action/bulk/TransportShardBulkAction.java
Lines 501 to 504 in 622a7f1
In the 0.0001% case, the immediate retry succeeds, causing the test to fail.
I was able to reproduce this bug about once every 10,000 tests. With the
awaitsFix I ran this 100,000 times with no failures.
Resolves #38428