Use soft-update to maintain document history by dnhatn · Pull Request #29458 · elastic/elasticsearch

dnhatn · 2018-04-10T20:17:22Z

Today we can use the soft-update feature from Lucene to maintain a history of document. This change simply replaces hard-update in the Engine by soft-update methods. Stale operations, delete, and no-ops will be handled in subsequent changes. This change is just a cut-over from hard-update to soft-update, no new functionality has been introduced.

Today we can the soft-update feature from Lucene to maintain a history of document. This change simply replaces hard-update in the Engine by soft-update methods. Stale operations, delete, and no-ops will be handled in subsequent changes.

elasticmachine · 2018-04-10T20:17:23Z

Pinging @elastic/es-distributed

dnhatn · 2018-04-10T22:00:02Z

test this please.

dnhatn · 2018-04-11T03:08:52Z

please test this.

s1monw · 2018-04-11T12:16:42Z

@elasticmachine test this please

s1monw

looks great, I left some comments

s1monw · 2018-04-11T12:17:12Z

server/src/main/java/org/elasticsearch/common/lucene/Lucene.java

+    /**
+     * Returns a numeric docvalues which can be used to soft-delete documents.
+     */
+    public static NumericDocValuesField getSoftDeleteDVMarker() {


call this newSoftDeletesField?

s1monw · 2018-04-11T12:17:46Z

server/src/main/java/org/elasticsearch/index/IndexSettings.java

+     * Returns <code>true</code> if soft-delete is enabled.
+     */
+    public boolean isSoftDeleteEnabled() {
+        return getIndexVersionCreated().onOrAfter(Version.V_7_0_0_alpha1) && softDeleteEnabled;


maybe resolve this in the ctor?

s1monw · 2018-04-11T12:18:34Z

server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java

+        if (softDeleteEnabled) {
+            iwc.setSoftDeletesField(Lucene.SOFT_DELETE_FIELD);
+            // TODO: soft-delete retention policy
+            mergePolicy = new SoftDeletesRetentionMergePolicy(Lucene.SOFT_DELETE_FIELD, () -> new MatchAllDocsQuery(), mergePolicy);


we don't need this here since it's match all. Lets just omit it until we use it

s1monw · 2018-04-11T12:20:19Z

server/src/main/java/org/elasticsearch/index/shard/ElasticsearchMergePolicy.java

        return delegate.useCompoundFile(segments, newSegment, writer);
    }

+    @Override


instead of doing this I think we should extend MergePolicyWrapper in master and 6.x and then this is handled upstream

s1monw · 2018-04-11T12:20:55Z

server/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java

            if (flush) {
                // we should have had just 1 merge, so last generation should be exact
-                assertEquals(gen2, store.readLastCommittedSegmentsInfo().getLastGeneration());
+                assertEquals(gen2 + 1, store.readLastCommittedSegmentsInfo().getLastGeneration());


hmm why does this need to change?

s1monw · 2018-04-11T12:21:07Z

server/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java

+        @Override
+        public long softUpdateDocument(Term term, Iterable<? extends IndexableField> doc, Field... softDeletes) throws IOException {
+            maybeThrowFailure();
+            return super.softUpdateDocument(term, doc, softDeletes);


s1monw · 2018-04-11T12:21:26Z

server/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java

+            public LeafReader wrap(LeafReader in) {
+                return new FilterLeafReader(in) {
+                    @Override
+                    public CacheHelper getCoreCacheHelper() {


this can return in.getCoreCacheHelper();

I think you can just move this entire thing to a util method in Lucene.java then we can reuse it in other places WDYT? @martijnvg you need this too somewhere right?

I removed the test and this wrapper in this PR, and will add it back to Lucene.java later. I prefer to keep this change as a cut-over.

s1monw · 2018-04-11T12:22:57Z

server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java

    private void updateDocs(final Term uid, final List<ParseContext.Document> docs, final IndexWriter indexWriter) throws IOException {
-        if (docs.size() > 1) {
-            indexWriter.updateDocuments(uid, docs);
+        if (softDeleteEnabled) {


I think in the delete case we should also use indexWriter.updateDocValue(uid, Lucene.getSoftDeleteDVMarker()); instead of IndexWriter#deleteDocuments and then override this in the test to maybe throw an exception.

dnhatn · 2018-04-11T19:00:17Z

@s1monw I have addressed your comments. Can you please take a look? Thank you.

s1monw

LGTM

s1monw · 2018-04-12T11:51:21Z

server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java

I think we can cache this in here in a constant so we don't need to re-create a new one every time.

dnhatn · 2018-04-12T16:30:19Z

Thanks @s1monw for reviewing.

Since elastic#29458, we use a searcher to calculate the number of documents for a commit stats. Sadly, that approach is flawed. The searcher might no longer point to the last commit if it's refreshed. This commit uses SoftDeletesDirectoryReaderWrapper to exclude the soft-deleted documents from numDocs in a SegmentInfos. I chose to modify the method Luence#getNumDocs so that we can read a store metadata snapshot correctly without opening an engine. Relates elastic#29458

Today we can use the soft-update feature from Lucene to maintain a history of document. This change simply replaces hard-update in the Engine by soft-update methods. Stale operations, delete, and no-ops will be handled in subsequent changes. This change is just a cut-over from hard-update to soft-update, no new functionality has been introduced. Relates #29530

Since #29458, we use a searcher to calculate the number of documents for a commit stats. Sadly, that approach is flawed. The searcher might no longer point to the last commit if it's refreshed. As synced-flush requires an exact numDocs to work correctly, we have to exclude all soft-deleted docs. This commit makes synced-flush stop using CommitStats but read an exact numDocs directly from an index commit. Relates #29458 Relates #29530

Use soft-update to maintain document history

f898440

Today we can the soft-update feature from Lucene to maintain a history of document. This change simply replaces hard-update in the Engine by soft-update methods. Stale operations, delete, and no-ops will be handled in subsequent changes.

dnhatn added >enhancement :Distributed/Engine Anything around managing Lucene and the Translog in an open shard. labels Apr 10, 2018

dnhatn requested review from bleskes and s1monw April 10, 2018 20:17

dnhatn requested review from jasontedor and martijnvg April 10, 2018 20:19

dnhatn added 2 commits April 10, 2018 20:27

only enable soft-delete in the latest version

c9d1fd1

Merge branch 'ccr' into ccr-soft-updates

6f3eb34

s1monw suggested changes Apr 11, 2018

View reviewed changes

dnhatn added 2 commits April 11, 2018 14:08

Merge branch 'ccr' into ccr-soft-updates

754b0e2

apply simonw feedbacks

90ab934

dnhatn requested a review from s1monw April 11, 2018 19:00

bleskes mentioned this pull request Apr 12, 2018

Introduce random-access translog #28205

Closed

s1monw approved these changes Apr 12, 2018

View reviewed changes

dnhatn added 2 commits April 12, 2018 09:36

Merge branch 'ccr' into ccr-soft-updates

82ef84d

Cache the soft-delete field

29cbece

dnhatn merged commit 0dd61fc into elastic:ccr Apr 12, 2018

dnhatn deleted the ccr-soft-updates branch April 12, 2018 16:30

dnhatn added the backport pending label Apr 12, 2018

dnhatn mentioned this pull request Apr 16, 2018

Use soft-deletes to maintain document history #29530

Closed

14 tasks

dnhatn mentioned this pull request Apr 28, 2018

Use exact numDocs in synced-flush and metadata snapshot #30228

Merged

dnhatn removed the backport pending label May 10, 2018

Conversation

dnhatn commented Apr 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Apr 10, 2018

Uh oh!

dnhatn commented Apr 10, 2018

Uh oh!

dnhatn commented Apr 11, 2018

Uh oh!

s1monw commented Apr 11, 2018

Uh oh!

s1monw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnhatn Apr 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnhatn commented Apr 11, 2018

Uh oh!

s1monw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnhatn commented Apr 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dnhatn commented Apr 10, 2018 •

edited

Loading

dnhatn Apr 11, 2018 •

edited

Loading