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 CAS deprecation to TransportShardBulkAction #42641

Merged
merged 9 commits into from
Jun 5, 2019

Conversation

dnhatn
Copy link
Member

@dnhatn dnhatn commented May 28, 2019

If the primary on 6.6+ but replicas on older versions, we can not use CAS using ifSeqNo since it requires all nodes on 6.6+. In this case, we have to fall back to use CAS with _version and should not issue a deprecation warning log during validation. This change moves the CAS deprecation to TransportShardBulkAction and only issues deprecation when the cluster is ready for ifSeqNo.

Closes #42796
Relates #42596

@dnhatn dnhatn added >non-issue :Distributed Indexing/Distributed A catch all label for anything in the Distributed Area. Please avoid if you can. v6.8.1 labels May 28, 2019
@dnhatn dnhatn requested a review from ywelsch May 28, 2019 16:09
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

@dnhatn
Copy link
Member Author

dnhatn commented May 28, 2019

@dnhatn
Copy link
Member Author

dnhatn commented May 28, 2019

@ywelsch This is ready. Please have a look. Thank you!

@dnhatn
Copy link
Member Author

dnhatn commented May 29, 2019

This change is not water tight. An update request can still fail if the primary shard which resolves the update request gets relocated before it executes the request while replicas are still on old nodes.

@ywelsch
Copy link
Contributor

ywelsch commented Jun 3, 2019

This change is not water tight. An update request can still fail if the primary shard which resolves the update request gets relocated before it executes the request while replicas are still on old nodes.

only in case of a single update request (TransportUpdateAction), not an update that's done as part of the bulk APIs (TransportShardBulkAction), right?

@ywelsch
Copy link
Contributor

ywelsch commented Jun 3, 2019

How about moving the deprecation logging that's currently done by DocWriteRequest.logDeprecationWarnings to TransportShardBulkAction and then only do it if all nodes are on 6.6+?

@dnhatn
Copy link
Member Author

dnhatn commented Jun 3, 2019

How about moving the deprecation logging that's currently done by DocWriteRequest.logDeprecationWarnings to TransportShardBulkAction and then only do it if all nodes are on 6.6+?

Yes, it would be a good option.

@dnhatn
Copy link
Member Author

dnhatn commented Jun 4, 2019

How about moving the deprecation logging that's currently done by DocWriteRequest.logDeprecationWarnings to TransportShardBulkAction and then only do it if all nodes are on 6.6+?

I made changes for this. However, there is still a case where we issue an unwanted deprecation warning for a single update request. If the cluster has an old node when TransportUpdateAction processes the request but that old node left when TransportShardBulkAction executes it.

How about moving this deprecation to the rest layer?

@ywelsch
Copy link
Contributor

ywelsch commented Jun 4, 2019

I made changes for this. However, there is still a case where we issue an unwanted deprecation warning for a single update request.

I think that's fine, this should be sufficient for our REST tests. It does not need to be perfect, but has to pass our tests.

How about moving this deprecation to the rest layer?

This means that requests from the transport client won't be logged, and also internal requests which mistakenly might use versioning would not be logged either. I prefer the previously proposed solution.

@dnhatn dnhatn changed the title Forgo deprecation when cluster not ready for ifSeqNo Move CAS deprecation to TransportShardBulkAction Jun 4, 2019
@dnhatn
Copy link
Member Author

dnhatn commented Jun 4, 2019

@ywelsch This is ready again. Can you please take a look? Thank you!

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

@dnhatn
Copy link
Member Author

dnhatn commented Jun 4, 2019

@elasticmachine test this please

@dnhatn
Copy link
Member Author

dnhatn commented Jun 5, 2019

@ywelsch Thanks for reviewing!

@dnhatn dnhatn merged commit 88767a2 into elastic:6.8 Jun 5, 2019
@dnhatn dnhatn deleted the cas-deprecation branch June 5, 2019 02:09
dnhatn added a commit that referenced this pull request Jun 6, 2019
Today CrudIT expects CAS deprecation warnings from
DocWriteRequest#validate which is executed locally on the client side
before sending the request. This suite is currently failing after we
moved the CAS deprecation check to TransportShardBulkAction (in #42641).
We need to adjust this suite to verify the CAS deprecation warnings from
the responses.

Closes #42881
dnhatn added a commit that referenced this pull request Jun 7, 2019
After we moved CAS deprecation check to TransportShardBulkAction
(#42641), a mixed cluster consisting of 6.7 and 6.8.1 nodes can issue
two deprecation warnings for versioning write requests. The way we
deduplicate response headers does not work for warning headers from
different versions. This commit skips CAS deprecation tests until all 
nodes on 6.8.1+.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed Indexing/Distributed A catch all label for anything in the Distributed Area. Please avoid if you can. >non-issue v6.8.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants