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

Version option deprecation fix and replacements #1803

Merged
merged 13 commits into from
Oct 15, 2020
Merged

Conversation

deluxetom
Copy link
Contributor

@deluxetom deluxetom commented Oct 6, 2020

As stated in #1796

  • removed version and version_type
  • added replacements if_seq_no and if_primary_term

@deguif
Copy link
Collaborator

deguif commented Oct 7, 2020

I'm still reading the changelog for 7.x and I'm not sure anymore what to do: https://www.elastic.co/guide/en/elasticsearch/reference/current/breaking-changes-7.0.html#breaking_70_api_changes

It clearly states:

Elasticsearch maintains a numeric version field for each document it stores. 
That field is incremented by one with every change to the document. 
Until 7.0.0 the API allowed using that field for optimistic concurrency control, i.e., making
a write operation conditional on the current document version. 
Sadly, that approach is flawed because the value of the version doesn’t always uniquely represent
a change to the document. If a primary fails while handling a write operation,
it may expose a version that will then be reused by the new primary.

Due to that issue, internal versioning can no longer be used and is replaced by a new method
based on sequence numbers. See Optimistic concurrency control for more details.

Note that the external versioning type is still fully supported.

There's also:

A number of duplicate parameters deprecated in 6.x have been removed from Bulk request, Multi Get request,
Term Vectors request, and More Like This Query requests.

The following camel case parameters have been removed:

- opType
- versionType, _versionType

The following parameters starting with underscore have been removed:

- _parent
- _retry_on_conflict
- _routing
- _version
- _version_type

Instead of these removed parameters, use their non camel case equivalents without starting underscore,
e.g. use version_type instead of _version_type or versionType.

Maybe @damienalexandre could help us with these changes in the api?

*
* @see https://www.elastic.co/guide/en/elasticsearch/reference/6.8/optimistic-concurrency-control.html
*/
public function setSequenceNumber($number)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
public function setSequenceNumber($number)
public function setSequenceNumber(int $number)

*
* @see https://www.elastic.co/guide/en/elasticsearch/reference/6.8/optimistic-concurrency-control.html
*/
public function setPrimaryTerm($term)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
public function setPrimaryTerm($term)
public function setPrimaryTerm(int $term)

}

/**
* Sets the prmary term of a document for use with optimistic concurrency control.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* Sets the prmary term of a document for use with optimistic concurrency control.
* Sets the primary term of a document for use with optimistic concurrency control.

*
* @return int|string Document version
*/
public function getPrimaryTerm()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
public function getPrimaryTerm()
public function getPrimaryTerm(): int

/**
* @return bool
*/
public function hasPrimaryTerm()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
public function hasPrimaryTerm()
public function hasPrimaryTerm(): bool

/**
* @return bool
*/
public function hasSequenceNumber()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
public function hasSequenceNumber()
public function hasSequenceNumber(): bool

*
* @return int|string Document version
*/
public function getSequenceNumber()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
public function getSequenceNumber()
public function getSequenceNumber(): int

Comment on lines -314 to -315
if (isset($responseData['_version'])) {
$data->setVersion($responseData['_version']);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure this should be removed finally. As it's still documented as returned.

Comment on lines -280 to -281
'version',
'version_type',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems the right fix as this not documented anymore: https://www.elastic.co/guide/en/elasticsearch/reference/7.x/docs-update.html

Comment on lines -218 to -219
if (isset($data['_version'])) {
$doc->setVersion($data['_version']);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure this should be removed finally. As it's still documented as returned.

@@ -272,10 +275,7 @@ public function getDocument($id, array $options = []): Document
$data = [];
}

$document = new Document($id, $data, $this->getName());
$document->setVersion($result['_version']);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure this should be removed finally. As it's still documented as returned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll put back setVersion, just remove it for a document update

src/Index.php Outdated
$document->setVersion($result['_version']);

return $document;
return new Document($id, $data, $this->getName());
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can probably add the seq_no and primary_term to document too as it's documented as returned: https://www.elastic.co/guide/en/elasticsearch/reference/7.9/docs-get.html#docs-get-api-response-body

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was debating doing that since the param for the document update is different, I think it needs to be separated between what we get back and what is sent for a document update

src/Client.php Outdated
Comment on lines 281 to 282
'if_primary_term',
'if_seq_no',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@deguif @ruflin after reading the docs, I think we should remove those from the default params. It shouldn't be forced since there are set when it gets a document right now

@deluxetom
Copy link
Contributor Author

@deguif @ruflin this is ready for review

Copy link
Collaborator

@deguif deguif left a comment

Choose a reason for hiding this comment

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

Many type-hints changes not in scope of this PR.
That would break the backward compatibility.

@@ -59,72 +59,105 @@ public function setIndex($index): self
*
* @return string Index name
*/
public function getIndex()
public function getIndex(): string
Copy link
Collaborator

Choose a reason for hiding this comment

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

BC break here, so should probably not be changed

*/
public function hasVersionType()
public function setVersion(int $version): self
Copy link
Collaborator

Choose a reason for hiding this comment

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

BC break too.

public function hasVersionType()
public function setVersion(int $version): self
{
return $this->setParam('_version', $version);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return $this->setParam('_version', $version);
return $this->setParam('version', $version);

@deluxetom
Copy link
Contributor Author

Many type-hints changes not in scope of this PR.
That would break the backward compatibility.

I reverted the type hints outside of the scope

Copy link
Owner

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

Is any of this going to be a breaking change for some of the users?

* @param int $version Document version
* @return $this
*/
public function setVersionParams(array $responseData): self
Copy link
Owner

Choose a reason for hiding this comment

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

Do we still need this method or could we remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its still used to set the document version on GET

if (isset($bulkResponseData['_version'])) {
$data->setVersion($bulkResponseData['_version']);
}
$data->setVersionParams($bulkResponseData);
Copy link
Owner

Choose a reason for hiding this comment

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

Now I see where we need this. Is there a better way to do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure there's a better way, I was trying to avoid to copy/paste code

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think copy/paste in this case is fine, adding another public method just for avoiding calling each setter doesn't bring so much value.

Copy link
Owner

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

LGTM.

@deguif Could you also give a 👍 or check if your concerns were addressed?

@deguif
Copy link
Collaborator

deguif commented Oct 14, 2020

@ruflin I'm not really confident with the impacts it can have but else I'm 👍

@ruflin
Copy link
Owner

ruflin commented Oct 15, 2020

@deguif That leaves a lot of room for interpretation 😆 The good news is, that this is master. I'll move forward with it.

@ruflin ruflin merged commit a6740da into ruflin:master Oct 15, 2020
ruflin pushed a commit that referenced this pull request Jan 18, 2022
As mentioned in #2049, the version/version_type has been removed in 7.x as concurrency control.

These options were still always added in the `addDocument` method which causes an issue when adding a document from one index to another.

This PR removes those options, and sorts the other options alphabetically cfr #1803.
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.

3 participants