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

Disallow VersionType.FORCE for GetRequest #21079

Merged
merged 1 commit into from
Nov 1, 2016

Conversation

dakrone
Copy link
Member

@dakrone dakrone commented Oct 21, 2016

This doesn't make much sense to have at all, since a user can do a GET
request without a version of they want to get it unconditionally.

Relates to #20995

This doesn't make much sense to have at all, since a user can do a `GET`
request without a version of they want to get it unconditionally.

Relates to elastic#20995
@dakrone dakrone added >breaking :Distributed/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search. v6.0.0-alpha1 labels Oct 21, 2016
@dakrone
Copy link
Member Author

dakrone commented Oct 21, 2016

This was missing from #20995, I didn't realize you could use force versioning with get requests until I did #21078, which is why it's a separate PR instead of in the original one.

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.

Looks good, but asked a question

@@ -101,6 +101,9 @@ public ActionRequestValidationException validate() {
validationException = ValidateActions.addValidationError("illegal version value [" + version + "] for version type [" + versionType.name() + "]",
validationException);
}
if (versionType == VersionType.FORCE) {
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't this a problem if coming from an update request with force? should we do something there to enable this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I dunno, the more I think about this, more I think we should not have a hard check on this, with 6.0 it won't break anything on GET requests and I'm not sure the complexity is worth it, it can just be removed in 7.0 entirely.

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

so to answer my own question - it's not a problem because the update request handling goes directly to the IndexShard - UpdateHelper#prepare(org.elasticsearch.action.update.UpdateRequest, org.elasticsearch.index.shard.IndexShard)

I think we should not have a hard check on this, with 6.0 it won't break anything on GET requests

I think this is still good to do? like we remove the FORCE support from other API in 6.0?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahh okay, if the UpdateHelper doesn't use this then we should be able to block this with no problem.

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.

now it's official :D

@dakrone dakrone merged commit eb4b6cd into elastic:master Nov 1, 2016
@dakrone dakrone deleted the disallow-force-get branch January 23, 2017 17:23
@clintongormley clintongormley changed the title Disallow VersionType.FORCE for GetRequest Disallow VersionType.FORCE for GetRequest May 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>breaking :Distributed/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search. v6.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants