Version option deprecation fix and replacements #1803

deluxetom · 2020-10-06T19:09:51Z

As stated in #1796

removed version and version_type
added replacements if_seq_no and if_primary_term

deguif · 2020-10-07T18:07:44Z

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?

deguif · 2020-10-07T18:39:02Z

src/AbstractUpdateAction.php

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


Suggested change

public function setSequenceNumber($number)

public function setSequenceNumber(int $number)

deguif · 2020-10-07T18:39:44Z

src/AbstractUpdateAction.php

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


Suggested change

public function setPrimaryTerm($term)

public function setPrimaryTerm(int $term)

deguif · 2020-10-07T18:40:03Z

src/AbstractUpdateAction.php

+    }
+
+    /**
+     * Sets the prmary term of a document for use with optimistic concurrency control.


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.

deguif · 2020-10-07T18:41:26Z

src/AbstractUpdateAction.php

+     *
+     * @return int|string Document version
+     */
+    public function getPrimaryTerm()


Suggested change

public function getPrimaryTerm()

public function getPrimaryTerm(): int

deguif · 2020-10-07T18:41:36Z

src/AbstractUpdateAction.php

+    /**
+     * @return bool
+     */
+    public function hasPrimaryTerm()


Suggested change

public function hasPrimaryTerm()

public function hasPrimaryTerm(): bool

deguif · 2020-10-07T18:41:52Z

src/AbstractUpdateAction.php

+    /**
+     * @return bool
+     */
+    public function hasSequenceNumber()


Suggested change

public function hasSequenceNumber()

public function hasSequenceNumber(): bool

deguif · 2020-10-07T18:41:59Z

src/AbstractUpdateAction.php

+     *
+     * @return int|string Document version
+     */
+    public function getSequenceNumber()


Suggested change

public function getSequenceNumber()

public function getSequenceNumber(): int

deguif · 2020-10-07T18:51:11Z

src/Client.php

-            if (isset($responseData['_version'])) {
-                $data->setVersion($responseData['_version']);


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

deguif · 2020-10-07T18:52:41Z

src/Client.php

-                    'version',
-                    'version_type',


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

deguif · 2020-10-07T18:53:10Z

src/Index.php

-            if (isset($data['_version'])) {
-                $doc->setVersion($data['_version']);


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

deguif · 2020-10-07T18:53:39Z

src/Index.php

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

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


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

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

deguif · 2020-10-07T18:56:03Z

src/Index.php

-        $document->setVersion($result['_version']);
-
-        return $document;
+        return new Document($id, $data, $this->getName());


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

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

deluxetom · 2020-10-07T20:19:16Z

src/Client.php

+                    'if_primary_term',
+                    'if_seq_no',


@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 · 2020-10-07T22:21:11Z

@deguif @ruflin this is ready for review

deguif

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

deguif · 2020-10-07T20:03:09Z

src/AbstractUpdateAction.php

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


BC break here, so should probably not be changed

deguif · 2020-10-08T07:37:02Z

src/AbstractUpdateAction.php

     */
-    public function hasVersionType()
+    public function setVersion(int $version): self


BC break too.

deguif · 2020-10-08T08:06:11Z

src/AbstractUpdateAction.php

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


Suggested change

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

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

deluxetom · 2020-10-08T12:53:44Z

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

ruflin

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

ruflin · 2020-10-12T11:20:47Z

src/AbstractUpdateAction.php

-     * @param int $version Document version
+     * @return $this
+     */
+    public function setVersionParams(array $responseData): self


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

its still used to set the document version on GET

ruflin · 2020-10-12T11:21:48Z

src/Bulk.php

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


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

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

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.

ruflin

LGTM.

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

deguif · 2020-10-14T19:08:52Z

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

ruflin · 2020-10-15T12:37:10Z

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

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.

Version option deprecation fix and replacements

cc5d010

deluxetom mentioned this pull request Oct 6, 2020

uncommented version in _params on updateDocument() to support Elatica… #1796

Closed

deluxetom added 4 commits October 7, 2020 11:38

Use new if_seq_no and if_primary_term

f1fc761

Changelog for if_seq_no and if_primary_term

0fae95e

Removed deprecated setVersion()

955d3ed

removed version from document

d15845e

deguif reviewed Oct 7, 2020

View reviewed changes

deluxetom added 3 commits October 7, 2020 15:45

type hints and keep set version from returned document

edccc89

fixed $bulkResponseData

ac3f4ad

only use _version

df38de8

deluxetom commented Oct 7, 2020

View reviewed changes

deluxetom added 3 commits October 7, 2020 16:30

optimized duplicate code

2254853

style fix

d794d2b

phpcs fixes

8863cee

deguif reviewed Oct 8, 2020

View reviewed changes

revert type hint changes

af0b1d5

deluxetom mentioned this pull request Oct 8, 2020

"version" is not a valid parameter #1797

Closed

ruflin reviewed Oct 12, 2020

View reviewed changes

Merge branch 'master' into master

1497fff

ruflin approved these changes Oct 14, 2020

View reviewed changes

ruflin merged commit a6740da into ruflin:master Oct 15, 2020

deguif added the hacktoberfest-accepted label Oct 24, 2020

deguif mentioned this pull request Dec 7, 2021

Elastica\Client\updateDocument fails with ES 7.3 #1683

Closed

This was referenced Jan 15, 2022

Internal versioning error when adding a document from one index to another #2049

Closed

Remove version/version_type options from Index::addDocument() #2050

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Version option deprecation fix and replacements #1803

Version option deprecation fix and replacements #1803

deluxetom commented Oct 6, 2020 •

edited

deguif commented Oct 7, 2020 •

edited

deguif Oct 7, 2020

deguif Oct 7, 2020

deguif Oct 7, 2020

deguif Oct 7, 2020

deguif Oct 7, 2020

deguif Oct 7, 2020

deguif Oct 7, 2020

deguif Oct 7, 2020

deguif Oct 7, 2020

deguif Oct 7, 2020

deguif Oct 7, 2020

deluxetom Oct 7, 2020

deguif Oct 7, 2020

deluxetom Oct 7, 2020

deluxetom Oct 7, 2020

deluxetom commented Oct 7, 2020

deguif left a comment

deguif Oct 7, 2020

deguif Oct 8, 2020

deguif Oct 8, 2020

deluxetom commented Oct 8, 2020

ruflin left a comment

ruflin Oct 12, 2020

deluxetom Oct 12, 2020

ruflin Oct 12, 2020

deluxetom Oct 12, 2020

deguif Oct 14, 2020

ruflin left a comment

deguif commented Oct 14, 2020

ruflin commented Oct 15, 2020

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

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

	* 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.

	public function getPrimaryTerm()
	public function getPrimaryTerm(): int

	public function hasPrimaryTerm()
	public function hasPrimaryTerm(): bool

	public function hasSequenceNumber()
	public function hasSequenceNumber(): bool

	public function getSequenceNumber()
	public function getSequenceNumber(): int

		if (isset($responseData['_version'])) {
		$data->setVersion($responseData['_version']);

		if (isset($data['_version'])) {
		$doc->setVersion($data['_version']);

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

Version option deprecation fix and replacements #1803

Version option deprecation fix and replacements #1803

Conversation

deluxetom commented Oct 6, 2020 • edited

deguif commented Oct 7, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deluxetom commented Oct 7, 2020

deguif left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deluxetom commented Oct 8, 2020

ruflin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruflin left a comment

Choose a reason for hiding this comment

deguif commented Oct 14, 2020

ruflin commented Oct 15, 2020

deluxetom commented Oct 6, 2020 •

edited

deguif commented Oct 7, 2020 •

edited