Use sequence numbers to identify out of order delivery in replicas & recovery by bleskes · Pull Request #24060 · elastic/elasticsearch

bleskes · 2017-04-12T06:48:51Z

Internal indexing requests in Elasticsearch may be processed out of order and repeatedly. This is important during recovery and due to concurrency in replicating requests between primary and replicas. As such, a replica/recovering shard needs to be able to identify that an incoming request contains information that is old and thus need not be processed. The current logic is based on external version. This is sadly not sufficient. This PR moves the logic to rely on sequences numbers and primary terms which give the semantics we need.

Relates to #10708

…refactoring alone"

s1monw

LGTM I left some minors that I would love to be addressed but no need for another round on my end.

s1monw · 2017-04-12T08:37:55Z

core/src/main/java/org/elasticsearch/common/lucene/uid/VersionsAndSeqNoResolver.java

+import static org.elasticsearch.common.lucene.uid.Versions.NOT_FOUND;
+
+/** Utility class to resolve the Lucene doc ID, version, seqNo and primaryTerms for a given uid. */
+public class VersionsAndSeqNoResolver {


make this class final? and does it need to be public?

s1monw · 2017-04-12T08:39:28Z

core/src/main/java/org/elasticsearch/common/lucene/uid/VersionsAndSeqNoResolver.java

+     * </ul>
+     */
+    public static DocIdAndVersion loadDocIdAndVersion(IndexReader reader, Term term) throws IOException {
+        assert term.field().equals(UidFieldMapper.NAME);


oh please give us a message here what field it is... it will save some CPU cycles if shit hits the fan

s1monw · 2017-04-12T08:39:48Z

core/src/main/java/org/elasticsearch/common/lucene/uid/VersionsAndSeqNoResolver.java

+     * </ul>
+     */
+    public static DocIdAndSeqNo loadDocIdAndSeqNo(IndexReader reader, Term term) throws IOException {
+        assert term.field().equals(UidFieldMapper.NAME);


s1monw · 2017-04-12T08:42:24Z

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

+        if (versionValue != null) {
+            if  (op.seqNo() > versionValue.getSeqNo() ||
+                (op.seqNo() == versionValue.getSeqNo() && op.primaryTerm() > versionValue.getTerm()))
+                status = OpVsLuceneDocStatus.OP_NEWER;


in general I find it odd that we have OP_STALE_OR_EQUAL instead of OP_NEWER_OR_EQUAL which is more natural then you have only one state where indexing would be fatal but I can see that this is an optimization to not index the doc again?

yes indeed. In the case of equality it should be identical and we can skip tokenization etc.

s1monw · 2017-04-12T08:45:35Z

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

            // a delete state and return false for the created flag in favor of code simplicity
-            final OpVsLuceneDocStatus opVsLucene = compareOpToLuceneDocBasedOnVersions(index);
+            final OpVsLuceneDocStatus opVsLucene;
+            if (index.seqNo() != SequenceNumbersService.UNASSIGNED_SEQ_NO) {


please leave a comment here when this unassigned seq no can occur... and when it can go away

I left an extra comment on the assertion in the else clause:

// This can happen if the primary is still on an old node and send traffic without seq# or we recover from translog // created by an old version. assert config().getIndexSettings().getIndexVersionCreated().before(Version.V_6_0_0_alpha1_UNRELEASED) : "index is newly created but op has no sequence numbers. op: " + index;

s1monw · 2017-04-12T08:46:48Z

core/src/main/java/org/elasticsearch/index/engine/VersionValue.java


-    VersionValue(long version) {
+    /** the seq number of the operation that last changed the associated uuid */
+    private final long seqNo;


we can also just use the members no need to getters... I like it when it's struct like for internal things like this. WDYT?

works for me. Less fluff is good.

bleskes · 2017-04-13T13:32:57Z

@s1monw all your feedback is addressed. Thx.

s1monw

still LGTM

jasontedor

This PR was very easy to read; I'm glad that it was factored out of the first PR for this reason. I left some very minor comments. It LGTM and I don't need to do another review.

jasontedor · 2017-04-14T17:09:34Z

core/src/main/java/org/elasticsearch/common/lucene/uid/PerThreadIDAndVersionSeqNoLookup.java

 *  finds. */

-final class PerThreadIDAndVersionLookup {
+final class PerThreadIDAndVersionSeqNoLookup {


The "and" is in an awkward place now, maybe PerThreadIDVersionAndSeqNoLookup?

agreed and fixed.

jasontedor · 2017-04-14T17:09:50Z

core/src/main/java/org/elasticsearch/common/lucene/uid/PerThreadIDAndVersionSeqNoLookup.java

    }
+
+    /** Return null if id is not found. */
+    public DocIdAndSeqNo lookupSequenceNo(BytesRef id, Bits liveDocs, LeafReaderContext context) throws IOException {


I think lookupSeqNo?

I think that this method can be package private? And maybe the other lookup methods in this class too?

agreed on all accounts.

jasontedor · 2017-04-14T17:11:45Z

core/src/main/java/org/elasticsearch/common/lucene/uid/VersionsAndSeqNoResolver.java

+            CloseableThreadLocal<PerThreadIDAndVersionSeqNoLookup> other = lookupStates.putIfAbsent(key, ctl);
+            if (other == null) {
+                // Our CTL won, we must remove it when the
+                // core is closed:


I think that you can put this comment all on one line.

I know it was like that before, but we are here now. 😄

jasontedor · 2017-04-14T17:11:55Z

core/src/main/java/org/elasticsearch/common/lucene/uid/VersionsAndSeqNoResolver.java

+                reader.addCoreClosedListener(removeLookupState);
+            } else {
+                // Another thread beat us to it: just use
+                // their CTL:


I think that you can put this comment all on one line.

I know it was like that before, but we are here now. 😄

jasontedor · 2017-04-14T17:40:15Z

core/src/main/java/org/elasticsearch/common/lucene/uid/PerThreadIDAndVersionSeqNoLookup.java

+        }
+    }
+
+    /** returns 0 if the primary term is not found */


Can you add link to IndexMetaData#primaryTerm as it has a note about the primary term on an operational shard always being strictly positive?

jasontedor · 2017-04-14T17:43:17Z

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

        LUCENE_DOC_NOT_FOUND
    }

+    private OpVsLuceneDocStatus compareOpToLuceneDocBasedOnSeqNo(final Operation op) throws IOException {


It's so nice to finally see this arriving.

jasontedor · 2017-04-14T17:43:49Z

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

    }

+    private OpVsLuceneDocStatus compareOpToLuceneDocBasedOnSeqNo(final Operation op) throws IOException {
+        assert op.seqNo() != SequenceNumbersService.UNASSIGNED_SEQ_NO : "resolving ops based seq# but no seqNo is found";


Nit: based -> based on

fixed. Thx.

jasontedor · 2017-04-14T17:45:39Z

core/src/main/java/org/elasticsearch/common/lucene/uid/PerThreadIDAndVersionSeqNoLookup.java

+    }
+
+    /** returns 0 if the primary term is not found */
+    public long lookUpPrimaryTerm(int docID) throws IOException {


Can it be package private?

jasontedor · 2017-04-14T17:46:21Z

core/src/main/java/org/elasticsearch/common/lucene/uid/VersionsAndSeqNoResolver.java

+        public final long version;
+        public final LeafReaderContext context;
+
+        public DocIdAndVersion(int docId, long version, LeafReaderContext context) {


I think that this constructor can be package private?

jasontedor · 2017-04-14T17:46:26Z

core/src/main/java/org/elasticsearch/common/lucene/uid/VersionsAndSeqNoResolver.java

+        public final long seqNo;
+        public final LeafReaderContext context;
+
+        public DocIdAndSeqNo(int docId, long seqNo, LeafReaderContext context) {


I think that this constructor can be package private?

jasontedor

LGTM. Punch it.

bleskes · 2017-04-14T19:46:43Z

Thx @jasontedor @s1monw

* master: Use sequence numbers to identify out of order delivery in replicas & recovery (elastic#24060) Remove customization of ES_USER and ES_GROUP

bleskes added 4 commits April 11, 2017 21:15

add failing tests

8b8e915

Revert "rollback seq no based out of order resolving and leave it at …

ee20e16

…refactoring alone"

versions can be final

825b8a1

assert text

3528877

bleskes added :Engine >enhancement v6.0.0-alpha1 labels Apr 12, 2017

bleskes requested review from jasontedor and s1monw April 12, 2017 06:48

bleskes mentioned this pull request Apr 12, 2017

Use sequence numbers to identify out of order delivery in replicas & recovery #23543

Closed

s1monw approved these changes Apr 12, 2017

View reviewed changes

feedback

bdd5a48

s1monw approved these changes Apr 13, 2017

View reviewed changes

jasontedor approved these changes Apr 14, 2017

View reviewed changes

bleskes added 2 commits April 14, 2017 21:05

Merge remote-tracking branch 'upstream/master' into seq_no_as_version_2

28cb7fb

feedback

a59a39a

jasontedor approved these changes Apr 14, 2017

View reviewed changes

bleskes merged commit ecf8168 into elastic:master Apr 14, 2017

bleskes deleted the seq_no_as_version_2 branch April 14, 2017 19:46

bleskes mentioned this pull request May 9, 2017

Add Sequence Numbers to write operations #10708

Closed

64 tasks

Conversation

bleskes commented Apr 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

bleskes commented Apr 13, 2017

Uh oh!

s1monw left a comment

Choose a reason for hiding this comment

Uh oh!

jasontedor 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!

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!

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!

jasontedor left a comment

bleskes commented Apr 12, 2017 •

edited

Loading