Guarantee that translog generations are seqNo conflict free #24825

bleskes · 2017-05-22T09:40:09Z

With #24779 in place, we can now guaranteed that a single translog generation file will never have a sequence number conflict that needs to be resolved by looking at primary terms. These conflicts can a occur when a replica contains an operation which isn't part of the history of a newly promoted primary. That primary can then assign a different operation to the same slot and replicate it to the replica.

PS. Knowing that each generation file is conflict free will simplifying repairing these conflicts when we read from the translog.

PPS. This PR also fixes some bugs in the piping of primary terms in the bulk shard action. These bugs are a result of the legacy of IndexRequest/DeleteRequest being a ReplicationRequest. We need to change that as a follow up.

Relates to #10708

…eqno

s1monw

left some minors LGTM in general

s1monw · 2017-05-22T12:24:45Z

core/src/main/java/org/elasticsearch/index/translog/Translog.java

@@ -1260,9 +1263,16 @@ public int hashCode() {
            return 31 * 31 * 31 + 31 * 31 * Long.hashCode(seqNo) + 31 * Long.hashCode(primaryTerm) + reason().hashCode();


this hashcode looks odd....

agreed. I generated a new one.

s1monw · 2017-05-22T12:25:26Z

core/src/main/java/org/elasticsearch/index/translog/TranslogWriter.java

@@ -90,6 +96,13 @@ private TranslogWriter(
        assert initialCheckpoint.maxSeqNo == SequenceNumbersService.NO_OPS_PERFORMED : initialCheckpoint.maxSeqNo;
        this.maxSeqNo = initialCheckpoint.maxSeqNo;
        this.globalCheckpointSupplier = globalCheckpointSupplier;
+        boolean assertionsEnabled = false;


assert (seenSequenceNumbers = new HashMap<>()) != null; if you wanna have a one-liner?

then I think I can't make seenSequenceNumbers final which is a shame...

Please integrate #24834 and use it here.

integrated. Nice work.

s1monw · 2017-05-22T12:28:22Z

core/src/main/java/org/elasticsearch/action/bulk/TransportShardBulkAction.java

-            DocWriteResponse primaryResponse,
-            IndexRequest request,
-            IndexShard replica) throws IOException {
+        DocWriteResponse primaryResponse,


can you maybe un-indent this into a single line?

s1monw · 2017-05-22T12:29:09Z

core/src/main/java/org/elasticsearch/index/shard/IndexShard.java

        try {
            verifyReplicationTarget();
+            assert primaryTerm == this.primaryTerm : "op term [ " + primaryTerm + " ] != shard term [" + this.primaryTerm + "]";


should this be a hard exception? something is bloody wrong here if this happens? maybe IllegalStateException?

this one is an interesting one - yes things are totally fucked up. I'm torn as to whether to change it - with assertions nodes die / test fail with the right exception as we never catch them. An IlegalStateException will cause the shard to be failed and then the failure may cascade further. I'm tempted to keep as is.

I think this should be kept as is exactly for the reason that @bleskes mentions regarding the consequences in production code of changing this to a hard failure and the possible loss of visibility when tests are running. This really should never happen.

s1monw · 2017-05-22T12:29:17Z

core/src/main/java/org/elasticsearch/index/shard/IndexShard.java

@@ -597,6 +598,7 @@ private IndexShardState changeState(IndexShardState newState, String reason) {
    public Engine.Delete prepareDeleteOnReplica(String type, String id, long seqNo, long primaryTerm,
                                                long version, VersionType versionType) {
        verifyReplicationTarget();
+        assert primaryTerm == this.primaryTerm : "op term [ " + primaryTerm + " ] != shard term [" + this.primaryTerm + "]";


s1monw · 2017-05-22T12:29:46Z

core/src/main/java/org/elasticsearch/index/shard/IndexShard.java

@@ -1879,8 +1881,9 @@ public void acquireReplicaOperationPermit(
                        indexShardOperationPermits.blockOperations(30, TimeUnit.MINUTES, () -> {
                            assert operationPrimaryTerm > primaryTerm;


can you add a message to this assertion while you are at it?

s1monw · 2017-05-22T12:46:45Z

core/src/main/java/org/elasticsearch/index/translog/TranslogWriter.java

        return new Translog.Location(generation, offset, data.length());
    }

+    private boolean assertSeqNoNotSeen(long seqNo, BytesReference data) throws IOException {


I know it's only called in one place but can you make it synchronized just for readability?

ywelsch

I've left minor comments around primary term but LGTM o.w.

ywelsch · 2017-05-22T17:37:36Z

core/src/main/java/org/elasticsearch/index/translog/Translog.java

@@ -1257,12 +1260,22 @@ public boolean equals(Object obj) {

        @Override
        public int hashCode() {
-            return 31 * 31 * 31 + 31 * 31 * Long.hashCode(seqNo) + 31 * Long.hashCode(primaryTerm) + reason().hashCode();
+            int result = (int) (seqNo ^ (seqNo >>> 32));
+            result = 31 * result + (int) (primaryTerm ^ (primaryTerm >>> 32));


result = 31 * result + Long.hashCode(primaryTerm); is simpler (id. for seqNo above)

another alternative would be to use Objects.hashCode(seqNo, primaryTerm, reason)

either way, we can avoid the bit-fiddling.

I just let IntelliJ autogen it.

ywelsch · 2017-05-22T17:41:06Z

core/src/main/java/org/elasticsearch/index/shard/IndexShard.java

                        });
-                    } catch (final InterruptedException | TimeoutException e) {
+                    } catch (final InterruptedException | TimeoutException | IOException | AlreadyClosedException e) {


maybe just catch (Exception)

yeah, this just got out of hand.

ywelsch · 2017-05-22T18:03:07Z

core/src/main/java/org/elasticsearch/index/shard/IndexShard.java

@@ -522,10 +522,11 @@ private IndexShardState changeState(IndexShardState newState, String reason) {
        }
    }

-    public Engine.Index prepareIndexOnReplica(SourceToParse source, long seqNo, long version, VersionType versionType, long autoGeneratedIdTimestamp,
-                                              boolean isRetry) {
+    public Engine.Index prepareIndexOnReplica(SourceToParse source, long seqNo, long primaryTerm, long version, VersionType versionType,


Calling this just primaryTerm is confusing (in light of a future PR that uses this code during resync). Here it means the term of the primary that was sending this operation, not necessarily the primary term of the log entry that is being replicated.

Here it means the term of the primary that was sending this operation, not necessarily the primary term of the log entry that is being replicated

Actually it's the term of the op in the log and not the authority of the primary that sends this op (that one gause into the permit method). I guess this proves your point about being confusing. Any suggestion?

I spoke to Yannick on a different channel. We decided to rename the parameter name opPrimaryTerm and relax the assertion to reflect semantics (as opposed to current usage).

ywelsch · 2017-05-22T18:25:53Z

core/src/main/java/org/elasticsearch/action/bulk/TransportShardBulkAction.java

@@ -667,7 +669,7 @@ static ReplicaItemExecutionMode replicaItemExecutionMode(final BulkItemRequest r
        final long version = primaryResponse.getVersion();
        assert versionType.validateVersionForWrites(version);
        final Engine.Delete delete = replica.prepareDeleteOnReplica(request.type(), request.id(),
-                primaryResponse.getSeqNo(), request.primaryTerm(), version, versionType);
+                primaryResponse.getSeqNo(), primaryTerm, version, versionType);


It's trappy that the IndexRequest/DeleteRequest object might not have a primaryTerm properly set if it's wrapped in a BulkShardRequest. Maybe we could override BulkShardRequest.primaryTerm(long) to set the primary term of the inner objects until we have fixed this discrepancy?

jasontedor

I left some comments.

jasontedor · 2017-05-22T16:24:27Z

core/src/main/java/org/elasticsearch/index/translog/Translog.java

-            return 31 * 31 * 31 + 31 * 31 * Long.hashCode(seqNo) + 31 * Long.hashCode(primaryTerm) + reason().hashCode();
+            int result = (int) (seqNo ^ (seqNo >>> 32));
+            result = 31 * result + (int) (primaryTerm ^ (primaryTerm >>> 32));
+            result = 31 * result + reason.hashCode();


Why change this? It was perfectly fine as it was, and far more readable. Note that the previous calculation was simply the result of expanding the usual hash code calculation:

accumulator = 1 for (o in objects) { accumulator = 31 * accumulator + hash(o) } return accumulator

So you end up with 31^k + 31^(k - 1) * hash_1 + 31^(k - 2) * hash_2 + ... 31^0 * hash_k hence why it looks like 31^3 + ...

I can roll it back. I just moved it to be the standard autogen-ed intellij code, which is what we see all over the place.

I'm just going to roll this back. It's not related to my changed and it seems it's contentious.

jasontedor · 2017-05-22T19:50:01Z

core/src/main/java/org/elasticsearch/index/translog/TranslogWriter.java

+        if (seqNo == SequenceNumbersService.UNASSIGNED_SEQ_NO) {
+            // nothing to do
+        } else if (seenSequenceNumbers.containsKey(seqNo)) {
+            final Tuple<BytesReference, Exception> previous = seenSequenceNumbers.get(seqNo);


Why not StackTraceElement[]?

I did it this way so it can easily integrate into a caused by clause and have a proper message. Do you have a better suggestion there?

jasontedor · 2017-05-22T19:50:18Z

core/src/main/java/org/elasticsearch/index/translog/TranslogWriter.java

+            }
+        } else {
+            seenSequenceNumbers.put(seqNo,
+                new Tuple<>(new BytesArray(data.toBytesRef(), true), new RuntimeException("stack capture previous op")));


Why not Thread.currentThread().getStackTrace()?

jasontedor · 2017-05-22T21:20:56Z

core/src/main/java/org/elasticsearch/index/translog/TranslogWriter.java

@@ -90,6 +96,13 @@ private TranslogWriter(
        assert initialCheckpoint.maxSeqNo == SequenceNumbersService.NO_OPS_PERFORMED : initialCheckpoint.maxSeqNo;
        this.maxSeqNo = initialCheckpoint.maxSeqNo;
        this.globalCheckpointSupplier = globalCheckpointSupplier;
+        boolean assertionsEnabled = false;


Please integrate #24834 and use it here.

jasontedor · 2017-05-22T21:22:41Z

core/src/main/java/org/elasticsearch/index/shard/IndexShard.java

@@ -522,10 +522,11 @@ private IndexShardState changeState(IndexShardState newState, String reason) {
        }
    }

-    public Engine.Index prepareIndexOnReplica(SourceToParse source, long seqNo, long version, VersionType versionType, long autoGeneratedIdTimestamp,
-                                              boolean isRetry) {
+    public Engine.Index prepareIndexOnReplica(SourceToParse source, long seqNo, long primaryTerm, long version, VersionType versionType,


jasontedor · 2017-05-22T21:24:02Z

core/src/main/java/org/elasticsearch/index/shard/IndexShard.java

        try {
            verifyReplicationTarget();
+            assert primaryTerm == this.primaryTerm : "op term [ " + primaryTerm + " ] != shard term [" + this.primaryTerm + "]";


I think this should be kept as is exactly for the reason that @bleskes mentions regarding the consequences in production code of changing this to a hard failure and the possible loss of visibility when tests are running. This really should never happen.

jasontedor

I left another comment.

jasontedor · 2017-05-23T02:59:07Z

core/src/main/java/org/elasticsearch/action/bulk/TransportShardBulkAction.java

-            DocWriteResponse primaryResponse,
-            IndexRequest request,
-            IndexShard replica) throws IOException {
+    private static Engine.IndexResult executeIndexRequestOnReplica(DocWriteResponse primaryResponse, IndexRequest request,


A reason that I prefer the line-per-parameter form for methods definitions that spill over multiple lines is because had you maintained it in this case the diff would simply be:

--- a/core/src/main/java/org/elasticsearch/action/bulk/TransportShardBulkAction.java +++ b/core/src/main/java/org/elasticsearch/action/bulk/TransportShardBulkAction.java @@ -531,6 +531,7 @@ public class TransportShardBulkAction extends TransportWriteAction<BulkShardRequ private static Engine.IndexResult executeIndexRequestOnReplica( DocWriteResponse primaryResponse, IndexRequest request, + long primaryTerm, IndexShard replica) throws IOException { final Engine.Index operation;

and git blame would trace that additional parameter back to this PR rather than the entire method declaration.

…eqno

bleskes · 2017-05-24T10:16:45Z

retest this please.

bleskes · 2017-05-24T11:27:01Z

thanks @s1monw @ywelsch @jasontedor

bleskes added 11 commits May 20, 2017 22:04

assert for duplicate seq# different content in seq no

01ea4a9

fix internal engine tests

1d88441

fix translog tests

0da7ded

fix IndexShardTests

6e3876a

bytes are tricky... clone you shall.

07d0076

improve Translog.Operation toString

e164882

better seqNo generation for replicas in IndexShardTestCase#indexDoc

3dc649d

add stack traces to capture

0dbfc3e

fix primary term mapping in delete

ec14051

fix primary term piping in a way that's bwc.

2c84f2c

Merge remote-tracking branch 'upstream/master' into translog_assert_s…

c3945b5

…eqno

bleskes added :Sequence IDs labels May 22, 2017

bleskes requested review from ywelsch and jasontedor May 22, 2017 09:40

bleskes added the v6.0.0 label May 22, 2017

s1monw approved these changes May 22, 2017

View reviewed changes

feedback

d3abba9

ywelsch approved these changes May 22, 2017

View reviewed changes

jasontedor reviewed May 22, 2017

View reviewed changes

jasontedor reviewed May 23, 2017

View reviewed changes

bleskes added 4 commits May 23, 2017 18:54

Merge remote-tracking branch 'upstream/master' into translog_assert_s…

9aac1f7

…eqno

use Assertions.ENABLED

797b11c

exceptions

d2a12c2

hash code

496d1c0

clintongormley added the >enhancement label May 23, 2017

bleskes added 3 commits May 24, 2017 08:40

Merge remote-tracking branch 'upstream/master' into translog_assert_s…

0ae4d11

…eqno

Merge remote-tracking branch 'upstream/master' into translog_assert_s…

984a03a

…eqno

primary term parameter naming and relaxed assertions

2ee3508

bleskes merged commit 6bc5b1d into elastic:master May 24, 2017

bleskes deleted the translog_assert_seqno branch May 24, 2017 11:26

bleskes mentioned this pull request May 29, 2017

Add Sequence Numbers to write operations #10708

Closed

64 tasks

clintongormley added v6.0.0-alpha2 and removed v6.0.0 labels Jun 6, 2017

		@@ -1260,9 +1263,16 @@ public int hashCode() {
		return 31 * 31 * 31 + 31 * 31 * Long.hashCode(seqNo) + 31 * Long.hashCode(primaryTerm) + reason().hashCode();

		@@ -1879,8 +1881,9 @@ public void acquireReplicaOperationPermit(
		indexShardOperationPermits.blockOperations(30, TimeUnit.MINUTES, () -> {
		assert operationPrimaryTerm > primaryTerm;

Guarantee that translog generations are seqNo conflict free #24825

Guarantee that translog generations are seqNo conflict free #24825

Conversation

bleskes commented May 22, 2017

s1monw 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

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

ywelsch 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

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

jasontedor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bleskes May 23, 2017 • edited Loading

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

jasontedor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bleskes commented May 24, 2017

bleskes commented May 24, 2017

bleskes May 23, 2017 •

edited

Loading