Avoid side-effect in VersionMap when assertion enabled #29585

dnhatn · 2018-04-18T14:29:30Z

Today when a version map does not require safe access, we will skip that document. However, if the assertion is enabled, we remove the delete tombstone of that document if existed. This side-effect may accidentally hide bugs in which stale delete tombstones can be accessed.

This change makes sure that putAssertionMap does not modify the tombstone maps.

Today when a version map does not require safe access, we will skip that document. However, if the assertion is enabled, we remove the delete tombstone of that document if existed. This may accidentally hide bugs in which stale delete tombstone can be accessed. This change folds a tombstone map into the version map so that each version map will have a separate tombstone map.

elasticmachine · 2018-04-18T14:29:32Z

Pinging @elastic/es-distributed

dnhatn · 2018-04-18T14:32:53Z

/cc @DaveCTurner

DaveCTurner

Looks like a good move. I suggested moving some methods around and a bit of renaming.

DaveCTurner · 2018-04-19T07:13:41Z

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

            this.current = current;
            this.old = old;
            this.previousMapsNeededSafeAccess = previousMapsNeededSafeAccess;
+            this.tombstones = tombstones; // transfer the tombstone


I think this comment isn't needed, WDYT?

DaveCTurner · 2018-04-19T07:21:42Z

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

-    /**
-     * Removes this uid from the pending deletes map.
-     */
+    // For testing


This -- and the number of times we do maps.tombstones.doStuff() -- feels a little strange. What do you think about moving methods like this onto Maps instead?

DaveCTurner · 2018-04-19T07:25:47Z

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

@@ -389,7 +397,8 @@ private boolean canRemoveTombstone(long maxTimestampToPrune, long maxSeqNoToPrun
     * Try to prune tombstones whose timestamp is less than maxTimestampToPrune and seqno at most the maxSeqNoToPrune.
     */
    void pruneTombstones(long maxTimestampToPrune, long maxSeqNoToPrune) {
-        for (Map.Entry<BytesRef, DeleteVersionValue> entry : tombstones.entrySet()) {
+        final Tombstones tombstones = maps.tombstones;


Similarly - this feels like it could move onto Maps or even Tombstones - AFAICT the only other thing it needs is maps.getMinDeleteTimestamp().

DaveCTurner · 2018-04-19T07:33:17Z

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

        }
+        assert putAssertionMap(uid, version);


I like that this has moved, but I think it changes things a little. The thing that's currently called unsafeKeysMap is now a proper copy of the version map that's always safe, so deserves renaming (and the comment above its field declaration needs fixing). Also I suspect getVersionForAssert could be changed to assert that the value in unsafeKeysMap is the value in the main map, when non-null.

bleskes

I'm +1 on not having the assertion logic in maybePutUnderLock change anything but the assertion map. I'm doubtful about how this is currently done. The current semantics of the extra map is just to keep operations that were skipped from the main map. Now it becomes a full copy and I'm not sure we need this. What we are really trying to solve is the maybePutUnderLock doesn't trim the tombstone, and for that we don't need a full shadow copy.

An alternative is to make maybePutUnderLock only accept a new IndexVersionValue and rename putUnderLock to deleteUnderLock which only accept DeleteVersionValue. Then each of these methods can do what they need to do - i.e., remove/add from the maps and add/remove from the tombstone. That way we can have dedicated paths to the two API and we will untangle the if in putUnderLock(BytesRef, VersionValue, Maps) . Then we won't touch the tombstone where we only intend to update the maps.

I think we can also rename TranslogVersionValue to IndexVersionValue instead of making a new one.

How do people feel about that? just a suggestion.

bleskes · 2018-04-19T08:10:19Z

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

-            long v = ramBytesUsedTombstones.addAndGet(-(BASE_BYTES_PER_CHM_ENTRY + prev.ramBytesUsed() + uidRAMBytesUsed));
-            assert v >= 0 : "bytes=" + v;
-        }
+        assert keyedLock.isHeldByCurrentThread(uid) : Thread.currentThread().getName();


while you're at it - can you add the uid?

This reverts commit 4107caa.

dnhatn · 2018-04-19T11:26:38Z

@bleskes and @DaveCTurner

The current semantics of the extra map is just to keep operations that were skipped from the main map. Now it becomes a full copy and I'm not sure we need this

I have restored this.

This reverts commit 81327ee.

bleskes · 2018-04-19T11:54:09Z

I have restored this.

I wanted to hear what you think of the idea. If you prefer what you did I'm fine too!

dnhatn · 2018-04-19T12:03:04Z

I wanted to hear what you think of the idea. If you prefer what you did I'm fine too!

@bleskes I am putting your idea into IntelliJ and will give you my thought.

bleskes

LGTM.

dnhatn · 2018-04-19T16:36:11Z

Thanks @bleskes and @DaveCTurner.

Today when a version map does not require safe access, we will skip that document. However, if the assertion is enabled, we remove the delete tombstone of that document if existed. This side-effect may accidentally hide bugs in which stale delete tombstone can be accessed. This change ensures putAssertionMap not modify the tombstone maps.

* master: Remove extra spaces from changelog Add support to match_phrase query for zero_terms_query. (elastic#29598) Fix incorrect references to 'zero_terms_docs' in query parsing error messages. (elastic#29599) Build: Move java home checks to pre-execution phase (elastic#29548) Avoid side-effect in VersionMap when assertion enabled (elastic#29585) [Tests] Remove accidental logger usage Add tests for ranking evaluation with aliases (elastic#29452) Deprecate use of `htmlStrip` as name for HtmlStripCharFilter (elastic#27429) Update plan for the removal of mapping types. (elastic#29586) [Docs] Add rankEval method for Jva HL client Make ranking evaluation details accessible for client

* master: (21 commits) Remove bulk fallback for write thread pool (elastic#29609) Fix an incorrect reference to 'zero_terms_docs' in match_phrase queries. Update the version compatibility for zero_terms_query in match_phrase. Account translog location to ram usage in version map Remove extra spaces from changelog Add support to match_phrase query for zero_terms_query. (elastic#29598) Fix incorrect references to 'zero_terms_docs' in query parsing error messages. (elastic#29599) Build: Move java home checks to pre-execution phase (elastic#29548) Avoid side-effect in VersionMap when assertion enabled (elastic#29585) [Tests] Remove accidental logger usage Add tests for ranking evaluation with aliases (elastic#29452) Deprecate use of `htmlStrip` as name for HtmlStripCharFilter (elastic#27429) Update plan for the removal of mapping types. (elastic#29586) [Docs] Add rankEval method for Jva HL client Make ranking evaluation details accessible for client Rename the bulk thread pool to write thread pool (elastic#29593) [Test] Minor changes to rank_eval tests (elastic#29577) Fix missing node id prefix in startup logs (elastic#29534) Added painless execute api. (elastic#29164) test: also assert deprecation warning after clusters have been closed. ...

Previously we did not put an indexing to a version map if that map does not require safe access but removed the existing delete tombstone only if assertion enabled. In #29585, we removed the side-effect caused by assertion then this test started failing. This failure can be explained as follows: - Step 1: Index a doc then delete that doc - Step 2: The version map can switch to unsafe mode because of concurrent refreshes (implicitly called by flushes) - Step 3: Index a document - the version map won't add this version value and won't prune the tombstone (previously it did) - Step 4: Delete a document - this will return NOT_FOUND instead of DELETED because of the stale delete tombstone This failure is actually fixed by #29619 in which we never leave stale delete tombstones Closes #29626

* es/master: (32 commits) TEST: Unmute testPrimaryRelocationWhileIndexing Remove remaining tribe node references (#29574) Never leave stale delete tombstones in version map (#29619) Do not serialize common stats flags using ordinal (#29600) Remove stale comment from JVM stats (#29625) TEST: Mute testPrimaryRelocationWhileIndexing Remove bulk fallback for write thread pool (#29609) Fix an incorrect reference to 'zero_terms_docs' in match_phrase queries. Update the version compatibility for zero_terms_query in match_phrase. Account translog location to ram usage in version map Remove extra spaces from changelog Add support to match_phrase query for zero_terms_query. (#29598) Fix incorrect references to 'zero_terms_docs' in query parsing error messages. (#29599) Build: Move java home checks to pre-execution phase (#29548) Avoid side-effect in VersionMap when assertion enabled (#29585) [Tests] Remove accidental logger usage Add tests for ranking evaluation with aliases (#29452) Deprecate use of `htmlStrip` as name for HtmlStripCharFilter (#27429) Update plan for the removal of mapping types. (#29586) [Docs] Add rankEval method for Jva HL client ...

* es/6.x: (28 commits) TEST: Unmute testPrimaryRelocationWhileIndexing Never leave stale delete tombstones in version map (#29619) Do not serialize common stats flags using ordinal (#29600) Remove stale comment from JVM stats (#29625) TEST: Mute testPrimaryRelocationWhileIndexing Remove 7.0.0 from 6.x changelog (#29621) Add support to match_phrase query for zero_terms_query. (#29598) Account translog location to ram usage in version map Avoid side-effect in VersionMap when assertion enabled (#29585) Build: Move java home checks to pre-execution phase (#29548) Add tests for ranking evaluation with aliases (#29452) [Test] Fix assertion in SearchDocumentationIT Deprecate use of `htmlStrip` as name for HtmlStripCharFilter (#27429) test: Assert deprecated http.enebled setting warning Update plan for the removal of mapping types. (#29586) [Docs] Add rankEval method for Jva HL client Make ranking evaluation details accessible for client Rename the bulk thread pool to write thread pool (#29593) [Test] Minor changes to rank_eval tests (#29577) test: Assert deprecated http.enebled setting warning ...

s1monw · 2018-04-20T07:17:30Z

thanks for fixing this!

dnhatn added >enhancement v7.0.0 v6.3.0 :Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. labels Apr 18, 2018

dnhatn requested review from s1monw and bleskes April 18, 2018 14:29

Always put into the assert map

4107caa

DaveCTurner reviewed Apr 19, 2018

View reviewed changes

bleskes reviewed Apr 19, 2018

View reviewed changes

Revert "Always put into the assert map"

32711aa

This reverts commit 4107caa.

Revert "Avoid side-effect in VersionMap when assertion enabled"

a7beda7

This reverts commit 81327ee.

dnhatn added 4 commits April 19, 2018 10:11

Untangle put index and put delete - Boaz’s suggestion

0346ba6

inline getVersionValue method

bec5ac3

Inline another test util method

e2b9f96

Do no remove tombstone when safe-access not required

516156a

bleskes approved these changes Apr 19, 2018

View reviewed changes

dnhatn merged commit 1b24d4e into elastic:master Apr 19, 2018

dnhatn deleted the assert-versionmap branch April 19, 2018 16:38

dnhatn added the backport pending label Apr 19, 2018

dnhatn removed the backport pending label Apr 19, 2018

dnhatn mentioned this pull request Apr 20, 2018

[CI] testPrimaryRelocationWhileIndexing fails on master branch #29626

Closed

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

DaveCTurner mentioned this pull request Feb 9, 2021

Don't allow descriptionless assertion statements in production code #68616

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid side-effect in VersionMap when assertion enabled #29585

Avoid side-effect in VersionMap when assertion enabled #29585

dnhatn commented Apr 18, 2018 •

edited

Loading

elasticmachine commented Apr 18, 2018

dnhatn commented Apr 18, 2018

DaveCTurner left a comment

DaveCTurner Apr 19, 2018

DaveCTurner Apr 19, 2018

DaveCTurner Apr 19, 2018

DaveCTurner Apr 19, 2018

bleskes left a comment

bleskes Apr 19, 2018

dnhatn commented Apr 19, 2018

bleskes commented Apr 19, 2018

dnhatn commented Apr 19, 2018

bleskes left a comment

dnhatn commented Apr 19, 2018

s1monw commented Apr 20, 2018

Avoid side-effect in VersionMap when assertion enabled #29585

Avoid side-effect in VersionMap when assertion enabled #29585

Conversation

dnhatn commented Apr 18, 2018 • edited Loading

elasticmachine commented Apr 18, 2018

dnhatn commented Apr 18, 2018

DaveCTurner left a comment

Choose a reason for hiding this comment

DaveCTurner Apr 19, 2018

Choose a reason for hiding this comment

DaveCTurner Apr 19, 2018

Choose a reason for hiding this comment

DaveCTurner Apr 19, 2018

Choose a reason for hiding this comment

DaveCTurner Apr 19, 2018

Choose a reason for hiding this comment

bleskes left a comment

Choose a reason for hiding this comment

bleskes Apr 19, 2018

Choose a reason for hiding this comment

dnhatn commented Apr 19, 2018

bleskes commented Apr 19, 2018

dnhatn commented Apr 19, 2018

bleskes left a comment

Choose a reason for hiding this comment

dnhatn commented Apr 19, 2018

s1monw commented Apr 20, 2018

dnhatn commented Apr 18, 2018 •

edited

Loading