Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Remove immediate operation retry after mapping update #38873

Merged
merged 1 commit into from
Feb 14, 2019
Merged

Remove immediate operation retry after mapping update #38873

merged 1 commit into from
Feb 14, 2019

Conversation

dakrone
Copy link
Member

@dakrone dakrone commented 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 #38428
Supercedes #38579
Relates to #38711

@dakrone
Remove immediate operation retry after mapping update
2331751 
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
@dakrone dakrone added :Distributed Indexing/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search. v7.0.0 v8.0.0 v7.2.0 labels Feb 13, 2019
@dakrone dakrone requested a review from bleskes February 13, 2019 23:10
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

@dakrone
Copy link
Member Author

dakrone commented Feb 13, 2019 •
edited
Loading

@bleskes this is the issue I talked to you about, there is more details about it on #38579 which solves one failure caused by it by waiting, I think this is a better solution though. Your name comes up with a refactor in #31821 so I thought you might want to take a look at it.

bleskes
bleskes approved these changes Feb 14, 2019
View reviewed changes
Copy link
Contributor

@bleskes bleskes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I can't think of any reason why not to do this and it's a good simplification. I'd like @ywelsch to sanity check this too.

PS. This doesn't guarantee that the cluster state is processed on all nodes when an indexing operation returns. You can still have a case where one node (that's not relevant for the indexing operation) didn't process the committed cluster state.

@dakrone dakrone mentioned this pull request Feb 14, 2019
Closed
@dakrone dakrone requested a review from ywelsch February 14, 2019 15:16
ywelsch
ywelsch approved these changes Feb 14, 2019
View reviewed changes
Copy link
Contributor

@ywelsch ywelsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks @dakrone

@dakrone dakrone merged commit 2a03802 into elastic:master Feb 14, 2019
dakrone added a commit that referenced this pull request Feb 14, 2019
@dakrone
Remove immediate operation retry after mapping update (#38873)
0c733c0 
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
@dakrone
Remove immediate operation retry after mapping update (#38873)
aff91b5 
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 dakrone added the v6.7.0 label Feb 14, 2019
dakrone added a commit that referenced this pull request Feb 14, 2019
@dakrone
Remove immediate operation retry after mapping update (#38873)
186a41b 
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 to dakrone/elasticsearch that referenced this pull request Feb 14, 2019
@dakrone
Revert "Mute failing test (elastic#38781)"
9785e2e 
This reverts commit b91e058.

This should be fixed by elastic#38873
@dakrone dakrone mentioned this pull request Feb 14, 2019
Merged
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Feb 14, 2019
@jasontedor
Merge remote-tracking branch 'elastic/master' into retention-lease-ccr
b8b2537 
* elastic/master:
  Remove immediate operation retry after mapping update (elastic#38873)
  Remove mentioning of types from bulk API docs (elastic#38896)
  SQL: change JDBC setup URL in the documentation (elastic#38564)
  Skip BWC tests in checkPart1 and checkPart2 (elastic#38730)
  Enable silent FollowersCheckerTest (elastic#38851)
  Update TESTING.asciidoc with platform specific instructions (elastic#38802)
  Use consistent view of realms for authentication (elastic#38815)
  Stabilize RareClusterState (elastic#38671)
  Increase Timeout in UnicastZenPingTests (elastic#38893)
  Do not recommend installing vagrant-winrm elastic#38887
  _cat/indices with Security, hide names when wildcard (elastic#38824)
  SQL: fall back to using the field name for column label (elastic#38842)
  Fix LocalIndexFollowingIT#testRemoveRemoteConnection() test (elastic#38709)
  Remove joda time mentions in documentation (elastic#38720)
  Add enabled status for token and api key service (elastic#38687)
@dakrone dakrone deleted the remove-immediate-index-retry branch February 14, 2019 18:03
pull bot pushed a commit to scher200/elasticsearch that referenced this pull request Feb 19, 2019
@dakrone
Revert "Mute failing test 20_mix_typless_typefull (elastic#38781)" (e…
f9d11ca 
…lastic#38912)

This reverts commit b91e058.

This should be fixed by elastic#38873

Resolves elastic#38711
dakrone added a commit to dakrone/elasticsearch that referenced this pull request Feb 19, 2019
@dakrone
Revert "Mute failing test 20_mix_typless_typefull (elastic#38781)" (e…
aec7a55 
…lastic#38912)

This reverts commit b91e058.

This should be fixed by elastic#38873

Resolves elastic#38711
dakrone added a commit to dakrone/elasticsearch that referenced this pull request Feb 19, 2019
@dakrone
Revert "Mute failing test 20_mix_typless_typefull (elastic#38781)" (e…
d0bc7a2 
…lastic#38912)

This reverts commit b91e058.

This should be fixed by elastic#38873

Resolves elastic#38711
This was referenced Feb 19, 2019
Merged
Merged
dakrone added a commit that referenced this pull request Feb 19, 2019
@dakrone
Revert "Mute failing test 20_mix_typless_typefull (#38781)" (#38912) (#…
41ac6f9 
…39141)

Backport of #38912

This reverts commit b91e058.

This should be fixed by #38873

Resolves #38711
dakrone added a commit that referenced this pull request Feb 19, 2019
@dakrone
Revert "Mute failing test 20_mix_typless_typefull (#38781)" (#38912) (#…
586db5e 
…39142)

Backport of #38912 

This reverts commit b91e058.

This should be fixed by #38873

Resolves #38711
@dakrone dakrone mentioned this pull request Feb 20, 2019
Merged
@danielmitterdorfer danielmitterdorfer added the >test Issues or PRs that are addressing/adding tests label Feb 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ywelsch ywelsch ywelsch approved these changes

@bleskes bleskes bleskes approved these changes

Assignees
No one assigned
Labels
:Distributed Indexing/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search. >test Issues or PRs that are addressing/adding tests v6.7.0 v7.0.0-rc2 v7.2.0 v8.0.0-alpha1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[CI] Failure in org.elasticsearch.index.mapper.DynamicMappingTests#testMappingVersionAfterDynamicMappingUpdate
6 participants
@dakrone @elasticmachine @bleskes @ywelsch @jakelandis @danielmitterdorfer