Add global checkpoint to translog checkpoints #21254

jasontedor · 2016-11-02T03:06:04Z

This commit adds the sequence number global checkpoint to translog
checkpoints, and removes them from Lucene commits.

This commit adds the sequence number global checkpoint to translog checkpoints, and removes them from Lucene commits.

bleskes

I like this. Left some comments where I think we need some more thinking (as we discussed). Thanks!

bleskes · 2016-11-02T12:52:13Z

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

                throw new IndexFormatTooOldException("trasnlog", "translog has no generation nor a UUID - this might be an index from a previous version consider upgrading to N-1 first");
            }
        }
-        final Translog translog = new Translog(translogConfig, generation);
+        final Translog translog = new Translog(translogConfig, generation, seqNoService::getGlobalCheckpoint);


nit - shall we make this a parameter to the method, like the other things?

I pushed 6cbed98.

bleskes · 2016-11-02T13:24:51Z

core/src/main/java/org/elasticsearch/index/seqno/SequenceNumbersService.java

-     * {@code globalCheckpoint} should be set to the last known global
-     * checkpoint for this shard, or
-     * {@link SequenceNumbersService#UNASSIGNED_SEQ_NO}.
+     * Initialize the sequence number service. The {@code maxSeqNo} should be set to the last sequence number assigned by this shard, or


OMG what happened here :)

bleskes · 2016-11-02T13:33:35Z

core/src/main/java/org/elasticsearch/index/seqno/SequenceNumbersService.java

@@ -155,6 +149,10 @@ public void updateAllocationIdsFromMaster(Set<String> activeAllocationIds, Set<S
     * of one of the active allocations is not known.
     */
    public boolean updateGlobalCheckpointOnPrimary() {
-        return globalCheckpointService.updateCheckpointOnPrimary();
+        final boolean maybeUpdateGlobalCheckpoint = globalCheckpointService.updateCheckpointOnPrimary();
+        if (maybeUpdateGlobalCheckpoint) {


I'm not sure this is what we want? maybeUpdateGlobalCheckpoint may be true because the global checkpoint isn't updated but we miss some local checkpoint. I think it's still ok to call this runnable, but we should document it if we keep it like this. Alternatively maybe we should split this method in two - updateGlobalCheckpointOnPrimary and needsLocalCheckpointSync and call those from IndexShard's updateGlobalCheckpointOnPrimary?

I've reverted the changes to this method, they are not needed after 86ec4d0.

bleskes · 2016-11-02T13:38:56Z

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

+        + Long.BYTES // generation
+        + CodecUtil.footerLength();
+
+    // TODO: remove legacy support, not needed in 6.0.0


make this a nocommit so we'll clean up later on?

I pushed 37a50bb.

bleskes · 2016-11-02T16:03:09Z

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

+    void onGlobalCheckpointUpdate() {
+        try (ReleasableLock ignored = readLock.acquire()) {
+            ensureOpen();
+            translog.sync();


since this all runs on an indexing thread, i'm concerned it will make us fsync twice. How about not fsync here but rather trigger a sync from the global checkpoint sync action?

I pushed 86ec4d0.

bleskes · 2016-11-02T16:10:50Z

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

+     * @return the sequence number stats
+     * @throws IOException if an I/O exception occurred reading the Lucene commit point or the translog checkpoint
+     */
+    private static SeqNoStats loadSeqNoStats(final EngineConfig engineConfig, final IndexWriter writer) throws IOException {


this is a little funky as we read some stuff from given parameters and some stuff from disk but then only if we're not supposed to ignore the translog. How about never reading the global checkpoint from the translog on opening and make it part of the recovery from translog? also it means peer recovery will need to also pass the global checkpoint as part the engine opening (but we can do this a follow up - add a no commit please if so)

I pushed 19d4db0.

This commit replaces a TODO on removing legacy support for non-checksummed translog checkpoints in Checkpoint.java with a nocommit.

This commit adds a nocommit to convert the way the global checkpoint is acquired when starting the engine to make it part of the recovery from the translog.

This commit causes the translog to be synced after every global checkpoint sync and removes syncing of the global checkpoint from the indexing path.

This commit modifies the signature of InternalEngine#openTranslog to feed through the global checkpoint supplier down to the opening of the translog.

jasontedor · 2016-11-07T17:22:59Z

Thanks @bleskes. I've responded to your feedback.

bleskes

LGTM, pending one testing question.

bleskes · 2016-11-08T11:32:20Z

core/src/main/java/org/elasticsearch/index/seqno/GlobalCheckpointSyncAction.java

        return new ReplicaResult();
    }

+    private void syncTranslog(final IndexShard indexShard) {


why do we need the conversion to an unchecked exceptions? shardOperationOnX Allows throwing them?

@bleskes This is correct for shardOperationOnPrimary (declares checked Exception) but not shardOperationOnReplica (does not declare any checked exceptions). I did push d742a68 since this was missing from the signature for GlobalCheckpointSyncAction#shardOperationOnPrimary.

In master both shardOperationOnPrimary and shardOperationOnReplica declare the top-level checked exception. We can pick this up and remove the wrapping after integrating master into feature/seq_no. I pushed 6c1338c.

bleskes · 2016-11-08T12:04:50Z

core/src/test/java/org/elasticsearch/index/translog/TranslogTests.java

@@ -136,7 +137,7 @@ public void tearDown() throws Exception {
    }

    private Translog create(Path path) throws IOException {
-        return new Translog(getTranslogConfig(path), null);
+        return new Translog(getTranslogConfig(path), null, () -> SequenceNumbersService.UNASSIGNED_SEQ_NO);


do we have a test that checks that the global checkpoint given by this supplier is properly persisted?

I pushed 37af724.

This commit adds a missing checked exception declared on TransportReplicationAction#shardOperationOnPrimary but not declared on the overridden method GlobalCheckpointSyncAction#shardOperationOnPrimary.

This commit adds a nocommit to GlobalCheckpointSyncAction to remove some exception wrapping after integrating master into feature/seq_no. This wrapping will not be needed after integration as TransportReplicationAction#shardOperationOnPrimary and TransportReplicationAction#shardOperationOnReplica will both declare a top-level checked exception.

This commit adds a low-level test that the supplied global checkpoint is persisted in translog checkpoints when the translog is synced.

jasontedor · 2016-11-08T14:20:09Z

@bleskes I pushed commits responding to your last round of feedback. Do you want to take another look?

This commit simplifies the advancement of the global checkpoint in TranslogTests#testBasicCheckpoint by moving the advancement into the test code.

bleskes · 2016-11-08T15:01:13Z

still LGTM. thx.

jasontedor · 2016-11-08T15:09:13Z

Thanks @bleskes.

Add global checkpoint to translog checkpoints

8bea450

This commit adds the sequence number global checkpoint to translog checkpoints, and removes them from Lucene commits.

jasontedor added >enhancement review v6.0.0-alpha1 labels Nov 2, 2016

jasontedor assigned bleskes Nov 2, 2016

bleskes suggested changes Nov 2, 2016

View reviewed changes

jasontedor mentioned this pull request Nov 3, 2016

Fix ShardInfo#toString #21319

Merged

jasontedor added 4 commits November 7, 2016 11:20

Replace TODO with nocommit in Checkpoint

37a50bb

This commit replaces a TODO on removing legacy support for non-checksummed translog checkpoints in Checkpoint.java with a nocommit.

Add nocommit for loading global checkpoint

19d4db0

This commit adds a nocommit to convert the way the global checkpoint is acquired when starting the engine to make it part of the recovery from the translog.

Sync translog after global checkpoint sync

86ec4d0

This commit causes the translog to be synced after every global checkpoint sync and removes syncing of the global checkpoint from the indexing path.

Pass global checkpoint supplier on open translog

6cbed98

This commit modifies the signature of InternalEngine#openTranslog to feed through the global checkpoint supplier down to the opening of the translog.

bleskes approved these changes Nov 8, 2016

View reviewed changes

jasontedor added 3 commits November 8, 2016 08:22

Add missing checked exception to GCSA override

d742a68

This commit adds a missing checked exception declared on TransportReplicationAction#shardOperationOnPrimary but not declared on the overridden method GlobalCheckpointSyncAction#shardOperationOnPrimary.

Add test for persistence of global checkpoint

37af724

This commit adds a low-level test that the supplied global checkpoint is persisted in translog checkpoints when the translog is synced.

Simplify global checkpoint advance in test

90f7b60

This commit simplifies the advancement of the global checkpoint in TranslogTests#testBasicCheckpoint by moving the advancement into the test code.

jasontedor merged commit 9ceb0f2 into elastic:feature/seq_no Nov 8, 2016

jasontedor deleted the global-checkpoint-in-translog branch November 8, 2016 15:09

bleskes mentioned this pull request Nov 11, 2016

Add Sequence Numbers to write operations #10708

Closed

64 tasks

jasontedor mentioned this pull request Dec 2, 2016

Clarify global checkpoint recovery #21934

Merged

clintongormley added :Engine :Distributed/Engine Anything around managing Lucene and the Translog in an open shard. and removed :Sequence IDs labels Feb 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add global checkpoint to translog checkpoints #21254

Add global checkpoint to translog checkpoints #21254

jasontedor commented Nov 2, 2016 •

edited by clintongormley

Loading

bleskes left a comment

bleskes Nov 2, 2016

jasontedor Nov 7, 2016

bleskes Nov 2, 2016

bleskes Nov 2, 2016

jasontedor Nov 7, 2016

bleskes Nov 2, 2016

jasontedor Nov 7, 2016

bleskes Nov 2, 2016

jasontedor Nov 7, 2016

bleskes Nov 2, 2016

jasontedor Nov 7, 2016

jasontedor commented Nov 7, 2016

bleskes left a comment

bleskes Nov 8, 2016

jasontedor Nov 8, 2016

jasontedor Nov 8, 2016

bleskes Nov 8, 2016

jasontedor Nov 8, 2016

jasontedor commented Nov 8, 2016

bleskes commented Nov 8, 2016

jasontedor commented Nov 8, 2016

Add global checkpoint to translog checkpoints #21254

Add global checkpoint to translog checkpoints #21254

Conversation

jasontedor commented Nov 2, 2016 • edited by clintongormley Loading

bleskes 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

jasontedor commented Nov 7, 2016

bleskes 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

jasontedor commented Nov 8, 2016

bleskes commented Nov 8, 2016

jasontedor commented Nov 8, 2016

jasontedor commented Nov 2, 2016 •

edited by clintongormley

Loading