Use Node Identifier, Fix ES Duplicates #378

remuslazar · 2021-04-10T11:56:28Z

Abstract

This changes the way the Elasticsearch document identifier is being calculated by using the node identifier instead of the node path.

Resolves

Moving a node leads to double-indexed nodes #353

Rationale

Using the node path to deduce the identifier in Elasticsearch has some flaws and bad side effects when moving nodes around; in that case, especially when using an async node indexing (e.g. by using the ES Queue Indexer), the indexing process will produce duplicates, basically it will leave the old node around and index a new node (with the correct path) and this will cause race conditions when doing ES queries and also issues when doing e.g. pagination etc.

Breaking

This change will require a "flow:nodeindex", so maybe a minor (or major?) release will the way to go.

Implementation Details

In addition to the logic change this also makes the existing implementation more DRY by removing the duplicated code in the IndexerDriver - see 1baf54e.

Testing

This also adds an implementation for the existing NodeIndexerTest::nodeMoveIsHandledCorrectly() test method.

To run the functional tests in e.g. docker, do:

FLOW_CONTEXT="Testing/ElasticVersion6" bin/phpunit --colors --stop-on-failure \
  -c Build/BuildEssentials/PhpUnit/FunctionalTests.xml \
  Packages/Application/Flowpack.ElasticSearch.ContentRepositoryAdaptor/Tests/Functional

Discussion

I will like to port this change to older versions as well so this will also work in older Neos 4.3 projects..

This adds the missing implementation for the existing nodeMoveIsHandledCorrectly method. Additionally this also add a new private method to get the node path as stored in ES.

This fixes a weird issue when the node instance from context does not have the right path. See inline comments for details.

Use the node identifier instead of its path to generate the ES document identifier

we want to use that in other places as well to stay DRY

Instead of doing the sha1 dance manually, use static method for that so we can stay DRY

baschny · 2021-04-12T16:28:33Z

👍 this has been working for us in a big site for the last 5 years successfully.

baschny · 2021-04-13T12:19:06Z

As a side-note: we (@cron-eu) will be using this in "5.0" branch for our use-case, so we could provide a PR with the fix even for that low version.

An idea to have this included would be to release it as "5.1.0" (and 6.1.0, 7.1.0, 8.1.0) and making a note that a "reindex" is required for that minor update. Don't know if one can expect people to read docs on that for a "simple patch level" release. And I don't think there is a "backwards compatible way" of fixing the bug without reindexing your content.

dfeyer

By reading the code, good job and nice move. Now we need to take a decision on the major or minor release strategy ;)

kdambekalns

Love this, left some comments about more explanation of things.

The release could be minor IMHO, and yes, I at least expect people to read things. 😎

And backporting that to older branches would be great, IMHO! 🚀

kdambekalns · 2021-04-23T06:04:41Z

Classes/Indexer/NodeIndexer.php

@@ -274,6 +274,13 @@ public function indexNode(NodeInterface $node, $targetWorkspaceName = null): voi
        $handleNode = function (NodeInterface $node, Context $context) use ($targetWorkspaceName, $indexer) {
            $nodeFromContext = $context->getNodeByIdentifier($node->getIdentifier());
            if ($nodeFromContext instanceof NodeInterface) {
+                if ($node->getPath() !== $nodeFromContext->getPath()) {
+                    // If the node from context does have a different path, purge the context cache and re-fetch


A note explaining when/why that could happen would be awesome!

see here my next comment on that topic.

kdambekalns · 2021-04-23T06:05:21Z

Classes/Indexer/NodeIndexer.php

+                if ($node->getPath() !== $nodeFromContext->getPath()) {
+                    // If the node from context does have a different path, purge the context cache and re-fetch
+
+                    // TODO: find a better way to handle this


What is wrong with this solution? Would you like to avoid flushing the full cache?

Indeed; I will prefer flushing the cache just for this specific node, but I was not able to figure out a good way to handle that. Do you have an idea here?

And maybe it should be safe(r) to just flush the cache always, regardless of the "node path was changed" condition. What do you think?

TL;DR

I can remember that back then, in 2016 (!) as I have implemented this initially, it took me some time of hair pulling to track down this issue.. I still do not quite understand why the cached data does not cause any issues when the node path was not being changed; maybe the neos context node caching behavior has some flaws, using the node path for invalidation or stuff like that?

dfeyer · 2021-11-18T09:09:55Z

Any news on this one ?

kdambekalns · 2021-11-18T16:29:28Z

I just had a quick look at the chanegs again and still like it. Nothing new, though. ;)

kdambekalns · 2021-11-18T16:30:31Z

Add @kitsunet as reviewer, lot's of ES usage "in his world", so maybe he can check.

daniellienert · 2021-11-18T16:40:58Z

Classes/Indexer/NodeIndexer.php


-        return sha1($contextPath);
+        return sha1($nodeIdentifier . $workspaceName);


Currently, the contextPath contains, site, workspace, dimensions, and nodeIdentifier.
Dimensions should/may be covered, as they are indexed into separate indices. But sites are currently combined in the indices per dimension. Could that be an issue when cloning a site where somehow the nodeIdentfiers are the same?

Why don't we just use the persistent_object_identifier, which is already unique?

If you have two sites with nodes sharing the same identifier, you are doomed, I think. That should never happen, no? Then again, it could happen, so adding the site here might be good.

Using the persistent_object_identifier OTOH is a no-go IMHO. It should stay an implementation detail internal to the CR and not be used outide it.

daniellienert · 2021-11-18T16:42:07Z

Classes/Driver/Version6/IndexerDriver.php

@@ -13,7 +13,7 @@
 * source code.
 */

-use Flowpack\ElasticSearch\ContentRepositoryAdaptor\Domain\Model\TargetContextPath;


This class should be obsolete now and should be removed.

dfeyer · 2022-04-22T14:51:27Z

@remuslazar Any news on this PR ? Should be nice to have it merged

daniellienert · 2022-05-04T14:04:06Z

Dear @remuslazar,

thanks for tackling this in the first place! During the core sprint, I reviewed and tested the change and it worked perfectly. We decided though, that we like to release the change as a minor version and so I implemented the feature as an alternative strategy in #394

I already merged that feature. And will close this one in favor of #394

baschny · 2022-05-04T18:54:38Z

@daniellienert great stuff, thanks a lot for your effort! 👍

remuslazar added 5 commits April 10, 2021 13:22

Implement nodeMoveIsHandledCorrectly test method

5922950

This adds the missing implementation for the existing nodeMoveIsHandledCorrectly method. Additionally this also add a new private method to get the node path as stored in ES.

Fix a caching issue in indexNode()

029fe51

This fixes a weird issue when the node instance from context does not have the right path. See inline comments for details.

Change the algorithm in calculateDocumentIdentifier

86ae087

Use the node identifier instead of its path to generate the ES document identifier

Make the calculateDocumentIdentifier method public and static

a6bf588

we want to use that in other places as well to stay DRY

Streamline the document identifier logic in IndexerDriver

1baf54e

Instead of doing the sha1 dance manually, use static method for that so we can stay DRY

remuslazar mentioned this pull request Apr 10, 2021

Moving a node leads to double-indexed nodes #353

Open

remuslazar added 2 commits April 10, 2021 14:00

Cleanup Code (fix code style issues)

f4f3b76

Cleanup Code (fix phpdoc)

8ef6c78

daniellienert requested review from daniellienert, kdambekalns and dfeyer April 10, 2021 12:19

remuslazar mentioned this pull request Apr 13, 2021

DAVPTA-623 Backport of "fix-node-move-issues" for 5.0 cron-eu/Flowpack.ElasticSearch.ContentRepositoryAdaptor#12

Merged

dfeyer approved these changes Apr 22, 2021

View reviewed changes

kdambekalns reviewed Apr 23, 2021

View reviewed changes

kdambekalns requested a review from kitsunet November 18, 2021 16:29

daniellienert requested changes Nov 18, 2021

View reviewed changes

daniellienert closed this May 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Node Identifier, Fix ES Duplicates #378

Use Node Identifier, Fix ES Duplicates #378

remuslazar commented Apr 10, 2021 •

edited

Loading

baschny commented Apr 12, 2021

baschny commented Apr 13, 2021

dfeyer left a comment

kdambekalns left a comment

kdambekalns Apr 23, 2021

remuslazar Apr 23, 2021

kdambekalns Apr 23, 2021

remuslazar Apr 23, 2021

dfeyer commented Nov 18, 2021

kdambekalns commented Nov 18, 2021

kdambekalns commented Nov 18, 2021

daniellienert Nov 18, 2021 •

edited

Loading

kdambekalns Nov 19, 2021

daniellienert Nov 18, 2021 •

edited

Loading

dfeyer commented Apr 22, 2022

daniellienert commented May 4, 2022

baschny commented May 4, 2022


		return sha1($contextPath);
		return sha1($nodeIdentifier . $workspaceName);

Use Node Identifier, Fix ES Duplicates #378

Use Node Identifier, Fix ES Duplicates #378

Conversation

remuslazar commented Apr 10, 2021 • edited Loading

Abstract

Resolves

Rationale

Breaking

Implementation Details

Testing

Discussion

baschny commented Apr 12, 2021

baschny commented Apr 13, 2021

dfeyer left a comment

Choose a reason for hiding this comment

kdambekalns left a comment

Choose a reason for hiding this comment

kdambekalns Apr 23, 2021

Choose a reason for hiding this comment

remuslazar Apr 23, 2021

Choose a reason for hiding this comment

kdambekalns Apr 23, 2021

Choose a reason for hiding this comment

remuslazar Apr 23, 2021

Choose a reason for hiding this comment

TL;DR

dfeyer commented Nov 18, 2021

kdambekalns commented Nov 18, 2021

kdambekalns commented Nov 18, 2021

daniellienert Nov 18, 2021 • edited Loading

Choose a reason for hiding this comment

kdambekalns Nov 19, 2021

Choose a reason for hiding this comment

daniellienert Nov 18, 2021 • edited Loading

Choose a reason for hiding this comment

dfeyer commented Apr 22, 2022

daniellienert commented May 4, 2022

baschny commented May 4, 2022

remuslazar commented Apr 10, 2021 •

edited

Loading

daniellienert Nov 18, 2021 •

edited

Loading

daniellienert Nov 18, 2021 •

edited

Loading