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.

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

Move watcher to use seq# and primary term for concurrency control #37977

Merged
merged 12 commits into from
Jan 31, 2019

Conversation

bleskes
Copy link
Contributor

@bleskes bleskes commented Jan 29, 2019

Watcher uses internal versioning to make sure a watch isn't changed when updating a status. This PR moves that logic to sequence numbers and opens up the option for users to also use seq# in the put watch API.

Relates #37872
Relates #10708

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features

@bleskes bleskes requested a review from jakelandis January 29, 2019 15:24
@bleskes bleskes requested a review from spinscale January 29, 2019 18:27
@@ -92,14 +94,20 @@ protected void doExecute(PutWatchRequest request, ActionListener<PutWatchRespons

if (isUpdate) {
UpdateRequest updateRequest = new UpdateRequest(Watch.INDEX, Watch.DOC_TYPE, request.getId());
updateRequest.version(request.getVersion());
if (request.getIfSeqNo() != SequenceNumbers.UNASSIGNED_SEQ_NO) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this one not guarded by

if (clusterService.state().nodes().getMinNodeVersion().onOrAfter(Version.V_7_0_0)) {

but the one in TransportAckWatchAction is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this one let's the user decide which feature they use. The one in the ack watch action is internal and needs to make a decision on their own.

Copy link
Contributor

Choose a reason for hiding this comment

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

makes sense

@spinscale
Copy link
Contributor

what about TransportActivateWatchAction?

TransportDeleteWatchAction does not use versioning because we always want to delete. I think this is fine in this case as well?

Copy link
Contributor

@spinscale spinscale left a comment

Choose a reason for hiding this comment

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

few questions for clarification, but in general it looks good

@bleskes
Copy link
Contributor Author

bleskes commented Jan 30, 2019

what about TransportActivateWatchAction?

I don't see any versioning support there. Am I missing something?

TransportDeleteWatchAction does not use versioning because we always want to delete. I think this is fine in this case as well?

That's what I was thinking.

@bleskes bleskes requested a review from spinscale January 30, 2019 14:37
Copy link
Contributor

@jakelandis jakelandis left a comment

Choose a reason for hiding this comment

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

Code LGTM. However, I think we may want to revisit keeping support for the version since I don't believe Kibana is currently using it (I could be wrong, based a quick test) and I could not find documentation on it's usage.

@bleskes bleskes merged commit b117321 into elastic:master Jan 31, 2019
@bleskes bleskes deleted the cas_watch branch January 31, 2019 01:15
@bleskes
Copy link
Contributor Author

bleskes commented Jan 31, 2019

Thx @spinscale & @jakelandis.

However, I think we may want to revisit keeping support for the version since I don't believe Kibana is currently using it

The idea is to remove it indeed once I backport this all.

jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jan 31, 2019
…r-primary-term

* elastic/master:
  Mute failing date index name processor test
  Reenable BWC testing after retention lease stats (elastic#38062)
  Move watcher to use seq# and primary term for concurrency control (elastic#37977)
  ILM setPriority corrections for a 0 value (elastic#38001)
  Temporarily disable BWC for retention lease stats (elastic#38049)
  Skip Shrink when numberOfShards not changed (elastic#37953)
  Add dispatching to `HandledTransportAction` (elastic#38050)
  Update httpclient for JDK 11 TLS engine (elastic#37994)
  Reduce flaxiness of ccr recovery timeouts test (elastic#38035)
  Fix ILM status to allow unknown fields (elastic#38043)
  Fix ILM Lifecycle Policy to allow unknown fields (elastic#38041)
  Update verify repository to allow unknown fields (elastic#37619)
  [ML] Datafeed deprecation checks (elastic#38026)
  Deprecate minimum_master_nodes (elastic#37868)
bleskes added a commit to bleskes/elasticsearch that referenced this pull request Feb 1, 2019
bleskes added a commit to bleskes/elasticsearch that referenced this pull request Feb 1, 2019
bleskes added a commit that referenced this pull request Feb 1, 2019
…37872 (#38155)

* Move update and delete by query to use seq# for optimistic concurrency control (#37857)

The delete and update by query APIs both offer protection against overriding concurrent user changes to the documents they touch. They currently are using internal versioning. This PR changes that to rely on sequences numbers and primary terms.

Relates #37639
Relates #36148
Relates #10708

* Add Seq# based optimistic concurrency control to UpdateRequest (#37872)

The update request has a lesser known support for a one off update of a known document version. This PR adds an a seq# based alternative to power these operations.

Relates #36148
Relates #10708

* Move watcher to use seq# and primary term for concurrency control (#37977)

* Adapt minimum versions for seq# power operations

After backporting #37977, #37857 and #37872
bleskes added a commit to bleskes/elasticsearch that referenced this pull request Feb 1, 2019
jasontedor pushed a commit that referenced this pull request Feb 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants