Advance checkpoints only after persisting ops by marregui · Pull Request #9309 · crate/crate

marregui · 2019-10-31T11:07:47Z

Do-over

Checklist

User relevant changes are recorded in CHANGES.txt
Touched code is covered by tests
Documentation has been updated if necessary
CLA is signed
This does not contain breaking changes, or if it does:
- It is released within a major release
- It is recorded in CHANGES.txt
- It was marked as deprecated in an earlier release if possible
- You've thought about the consequences and other components are adapted
  (E.g. AdminUI)

marregui · 2019-10-31T16:37:15Z

retest this please

cannot reproduce locally

mfussenegger

I think we should also port over LocalCheckpointTrackerTests

mfussenegger · 2019-11-04T09:22:21Z

es/es-server/src/main/java/org/elasticsearch/index/engine/Engine.java

+        public boolean exists() {
+            return exists;
+        }
+
+        public long version() {
+            return this.version;
+        }


Can we remove these extra properties again? See 836b1bb for why they were removed.

mfussenegger · 2019-11-04T09:23:08Z

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

+                translog = openTranslog(engineConfig, translogDeletionPolicy, engineConfig.getGlobalCheckpointSupplier(),
+                    seqNo -> {
+                        final LocalCheckpointTracker tracker = getLocalCheckpointTracker();
+                        assert tracker != null || getTranslog().isOpen() == false;


Could we add some assertion message here?

mfussenegger · 2019-11-04T09:24:01Z

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

+            SequenceNumbers.loadSeqNoInfoFromLuceneCommit(store.readLastCommittedSegmentsInfo().userData.entrySet());
+        maxSeqNo = seqNoStats.maxSeqNo;
+        localCheckpoint = seqNoStats.localCheckpoint;
+        logger.trace("recovered maximum sequence number [{}] and local checkpoint [{}]", maxSeqNo, localCheckpoint);


maybe wrap in logger.isTraceEnabled

mfussenegger · 2019-11-04T09:24:55Z

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

+                localCheckpointTracker.markSeqNoAsProcessed(indexResult.getSeqNo());
+                if (indexResult.getTranslogLocation() == null) {
+                    // the op is coming from the translog (and is hence persisted already) or it does not have a sequence number
+                    assert index.origin().isFromTranslog() || indexResult.getSeqNo() == SequenceNumbers.UNASSIGNED_SEQ_NO;


Same here, an assertion message would be nice

mfussenegger · 2019-11-04T09:25:12Z

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

+            localCheckpointTracker.markSeqNoAsProcessed(deleteResult.getSeqNo());
+            if (deleteResult.getTranslogLocation() == null) {
+                // the op is coming from the translog (and is hence persisted already) or does not have a sequence number (version conflict)
+                assert delete.origin().isFromTranslog() || deleteResult.getSeqNo() == SequenceNumbers.UNASSIGNED_SEQ_NO;


marregui · 2019-11-04T10:56:35Z

retest this please

I cannot reproduce locally....

marregui · 2019-11-04T11:59:32Z

retest this please

seems flaky, I am investigating locally

marregui · 2019-11-04T13:26:25Z

retest this please

it is flaky, and I cannot reproduce it locally. Taking the one from ES requires some changes, some code to import too

mfussenegger

otherwise lgtm

mfussenegger · 2019-11-11T08:52:21Z

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

+    @Seed("EA3633FB2FE61702")
+    @Repeat(iterations = 1)


I guess this should be reverted now

mfussenegger · 2019-11-11T08:52:32Z

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

+    @Seed("99FCB15FF5D32AD0")
+    @Repeat(iterations = 1)


Port of elastic/elasticsearch#43205 Local and global checkpoints currently do not correctly reflect what's persisted to disk. The issue is that the local checkpoint is adapted as soon as an operation is processed (but not fsynced yet). This leaves room for the history below the global checkpoint to still change in case of a crash. As we rely on global checkpoints for CCR as well as operation-based recoveries, this has the risk of shard copies / follower clusters going out of sync.

…VerifyShardBeforeCloseAction #9309 ports over elastic/elasticsearch#43205 but at that point TransportVerifyShardBeforeCloseAction was not present in our code base.

…ifyShardBeforeCloseAction #9309 ports over elastic/elasticsearch#43205 but at that point TransportVerifyShardBeforeCloseAction was not present in our code base.

marregui force-pushed the ma/port_PR43205 branch 3 times, most recently from 51bc1f1 to b858f63 Compare October 31, 2019 16:07

marregui requested a review from mfussenegger October 31, 2019 16:09

marregui requested a review from seut November 1, 2019 08:54

mfussenegger reviewed Nov 4, 2019

View reviewed changes

marregui force-pushed the ma/port_PR43205 branch from b858f63 to 7806c0c Compare November 4, 2019 10:33

marregui requested a review from mfussenegger November 4, 2019 12:32

mfussenegger approved these changes Nov 4, 2019

View reviewed changes

marregui force-pushed the ma/port_PR43205 branch from 7806c0c to 02f7648 Compare November 4, 2019 13:00

marregui added the ready-to-merge Let Mergify merge the PR once approved and checks pass label Nov 4, 2019

mfussenegger removed the ready-to-merge Let Mergify merge the PR once approved and checks pass label Nov 4, 2019

marregui force-pushed the ma/port_PR43205 branch from 02f7648 to eb6087d Compare November 7, 2019 17:22

marregui requested a review from mfussenegger November 8, 2019 20:21

mfussenegger approved these changes Nov 11, 2019

View reviewed changes

marregui force-pushed the ma/port_PR43205 branch from 5759c0d to 71294dd Compare November 11, 2019 10:03

marregui added the ready-to-merge Let Mergify merge the PR once approved and checks pass label Nov 11, 2019

mergify bot merged commit dfed1ca into master Nov 11, 2019

mergify bot deleted the ma/port_PR43205 branch November 11, 2019 12:23

marregui mentioned this pull request Dec 4, 2019

Rebuild version map when opening internal engine (bp #9394) #9419

Merged

marregui self-assigned this Dec 4, 2019

marregui added the v/4.0 label Dec 5, 2019

mergify bot mentioned this pull request Dec 5, 2019

Advance checkpoints only after persisting ops (bp #9309) #9431

Merged

Conversation

marregui commented Oct 31, 2019

Checklist

Uh oh!

marregui commented Oct 31, 2019

Uh oh!

mfussenegger left a comment

Choose a reason for hiding this comment

Uh oh!

mfussenegger Nov 4, 2019

Choose a reason for hiding this comment

Uh oh!

mfussenegger Nov 4, 2019

Choose a reason for hiding this comment

Uh oh!

mfussenegger Nov 4, 2019

Choose a reason for hiding this comment

Uh oh!

mfussenegger Nov 4, 2019

Choose a reason for hiding this comment

Uh oh!

mfussenegger Nov 4, 2019

Choose a reason for hiding this comment

Uh oh!

marregui commented Nov 4, 2019

Uh oh!

marregui commented Nov 4, 2019

Uh oh!

marregui commented Nov 4, 2019

Uh oh!

mfussenegger left a comment

Choose a reason for hiding this comment

Uh oh!

mfussenegger Nov 11, 2019

Choose a reason for hiding this comment

Uh oh!

mfussenegger Nov 11, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants