Move GlobalCheckpointTracker and remove SequenceNumbersService by ywelsch · Pull Request #27837 · elastic/elasticsearch

ywelsch · 2017-12-15T10:48:43Z

This PR moves GlobalCheckpointTracker from the engine to IndexShard, where it better fits logically: Tracking the global checkpoint based on the local checkpoints of all shards in the replication group is not a property of the engine, but rather a property fulfilled by the current primary shard. The LocalCheckpointTracker on the other hand is driven by the contents of the local translog. By moving GlobalCheckpointTracker to IndexShard, it makes little sense to keep the SequenceNumbersService class around - it would only wrap the LocalCheckpointTracker. This PR therefore removes the class and replaces occurrences of SequenceNumbersService in the engine directly by LocalCheckpointTracker.

bleskes · 2017-12-15T12:50:18Z

I like this very much. I think we should do it but also take it a step further and total isolate the GlobalCheckpointTracker from the engine. Something like this, wdyt?

patch.txt

bleskes

LGTM. Awesome stats.

bleskes · 2017-12-16T18:38:14Z

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

+        BiFunction<Long, Long, LocalCheckpointTracker> localCheckpointTrackerSupplier) throws IOException {
+        final long maxSeqNo;
+        final long localCheckpoint;
+        if (openMode == EngineConfig.OpenMode.CREATE_INDEX_AND_TRANSLOG) {


paranoia - can you use a switch statement with a fallback logic and an exception on default?

done in ea23a0d

bleskes · 2017-12-16T18:40:57Z

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

+
        final EngineConfig config = newEngineConfig(openMode);
+
+        if (openMode == EngineConfig.OpenMode.OPEN_INDEX_AND_TRANSLOG) {


can you add a comment why it's important to do it before we open the engine?

sure, added in ea23a0d

bleskes · 2017-12-16T18:42:01Z

core/src/main/java/org/elasticsearch/index/store/Store.java

     * @throws IOException if an I/O exception occurred reading the latest Lucene commit point from disk
     */
-    public SeqNoStats loadSeqNoStats(final long globalCheckpoint) throws IOException {
+    public Tuple<Long, Long> loadSeqNoStats() throws IOException {


maybe call this load seqNoInfo? This way people won't be confused about SeqNoStats?

bleskes · 2017-12-16T18:43:41Z

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

     *                              With no readers and no current, a call to  {@link #getMinFileGeneration()} would not work.
     */
-    private TranslogWriter createWriter(long fileGeneration, long initialMinTranslogGen) throws IOException {
+    private TranslogWriter createWriter(long fileGeneration, long initialMinTranslogGen, long initialGlobalCheckpoint) throws IOException {


java doc update?

bleskes · 2017-12-16T18:43:59Z

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

+            final LongSupplier writerGlobalCheckpointSupplier;
+            if (Assertions.ENABLED) {
+                writerGlobalCheckpointSupplier = () -> {
+                    long gcp = globalCheckpointSupplier.getAsLong();


bleskes · 2017-12-16T18:46:00Z

core/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java

-        engine = new InternalEngine(config(defaultSettings, store, translogPath, newMergePolicy(), null, null), seqNoServiceSupplier);
+        final AtomicLong globalCheckpoint = new AtomicLong(SequenceNumbers.UNASSIGNED_SEQ_NO);
+        final LongSupplier globalCheckpointSupplier = () -> globalCheckpoint.get();
+        engine = new InternalEngine(config(defaultSettings, store, translogPath, newMergePolicy(), null, null, globalCheckpointSupplier));


bleskes · 2017-12-16T18:47:34Z

core/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java

                Long.parseLong(initialEngine.commitStats().getUserData().get(SequenceNumbers.LOCAL_CHECKPOINT_KEY)),
                equalTo(localCheckpoint));
            initialEngine.getTranslog().sync(); // to guarantee the global checkpoint is written to the translog checkpoint
-            assertThat(


why is this gone?

I found it tricky to keep these checks in the test with the other changes around Engine not exposing GlobalCheckpointTracker. I also did not like that this test was checking a mixture between local checkpoint and global checkpoint properties (where the global checkpoint properties are mostly trivial, and covered by GlobalCheckpointTrackerTests). I've found a way to reintroduce these checks though (see ea23a0d)

bleskes · 2017-12-16T18:49:02Z

test/framework/src/main/java/org/elasticsearch/index/engine/EngineTestCase.java

                config.getExternalRefreshListener(), Collections.emptyList(), config.getIndexSort(), config.getTranslogRecoveryRunner(),
-                config.getCircuitBreakerService());
+                config.getCircuitBreakerService(), config.getGlobalCheckpointSupplier());
+                //new GlobalCheckpointTracker(config.getShardId(), config.getAllocationId(),


left overs?

right -> removed

bleskes · 2017-12-16T18:49:23Z

test/framework/src/main/java/org/elasticsearch/index/shard/IndexShardTestCase.java

        return indexShard.getEngine();
    }
+
+    public static GlobalCheckpointTracker getGlobalCheckpointTracker(IndexShard indexShard) {


ywelsch · 2017-12-18T14:59:42Z

backport pending waiting on backport of #27826

The PR #27837 unintentionally changed to an in memory global checkpoint.

Relates #27837

This commit moves GlobalCheckpointTracker from the engine to IndexShard, where it better fits logically: Tracking the global checkpoint based on the local checkpoints of all shards in the replication group is not a property of the engine, but rather a property fulfilled by the current primary shard. The LocalCheckpointTracker on the other hand is driven by the contents of the local translog. By moving GlobalCheckpointTracker to IndexShard, it makes little sense to keep the SequenceNumbersService class around - it would only wrap the LocalCheckpointTracker. This commit therefore removes the class and replaces occurrences of SequenceNumbersService in the engine directly by LocalCheckpointTracker.

dnhatn · 2017-12-20T01:56:28Z

Backported to 6.x in 1576a2b

* es/6.x: (43 commits) ingest: upgraded ingest geoip's geoip2's dependencies. [TEST] logging for update by query test #27820 Use full profile on JDK 10 builds Require Gradle 4.3 Add unreleased v6.1.2 version TEST: reduce blob size #testExecuteMultipartUpload Check index under the store metadata lock (#27768) Upgrade to Lucene 7.2.0. (#27910) Fixed test to be up to date with the new database files. Use `_refresh` to shrink the version map on inactivity (#27918) Make KeyedLock reentrant (#27920) Fixes DocStats to not report index size < -1 (#27863) Disable TestZenDiscovery in cloud providers integrations test ingest: Upgraded the geolite2 databases. [Issue-27716]: CONTRIBUTING.md IntelliJ configurations settings are confusing. (#27717) [Test] Fix IndicesClientDocumentationIT (#27899) Move uid lock into LiveVersionMap (#27905) Mute testRetentionPolicyChangeDuringRecovery Increase Gradle heap space to 1536m Move GlobalCheckpointTracker and remove SequenceNumbersService (#27837) ...

Today we did not set the global checkpoint when opening an engine from an existing store. If we are forced to close an engine before advancing the global checkpoint, we also have to close translog which in turn sync a new checkpoint with an unassigned global checkpoint. This is not caught until the global checkpoint assertion was introduced in PR elastic#27837. This commit tightens the syncNeeded conditions. Relates elastic#27970

ywelsch added 3 commits December 14, 2017 18:22

Remove SequenceNumbersService

2750fd8

fix tests

e0ede9c

fix javadoc

c79ea69

ywelsch added :Sequence IDs >non-issue v6.2.0 v6.3.0 v7.0.0 labels Dec 15, 2017

ywelsch requested a review from bleskes December 15, 2017 10:48

ywelsch added 3 commits December 15, 2017 16:51

Remove GlobalCheckpointTracker from EngineConfig

48747b2

Merge remote-tracking branch 'elastic/master' into remove-seq-no-service

9c18671

remove one more method

b954aac

bleskes approved these changes Dec 16, 2017

View reviewed changes

ywelsch added 8 commits December 18, 2017 09:16

Merge remote-tracking branch 'elastic/master' into remove-seq-no-service

a064c09

address review comments

ea23a0d

fix test

9b5fff7

fix another test

6795c8b

fix visibility

2a85aac

fix javadoc

369722a

Merge remote-tracking branch 'elastic/master' into remove-seq-no-service

cc957e5

thanks @bleskes

e1d45ab

ywelsch merged commit a5e8a22 into elastic:master Dec 18, 2017

ywelsch added the backport pending label Dec 18, 2017

dnhatn added a commit that referenced this pull request Dec 18, 2017

Put back lastSyncedGlobalCheckpoint in deletion policy

0d99fad

The PR #27837 unintentionally changed to an in memory global checkpoint.

dnhatn added a commit that referenced this pull request Dec 19, 2017

Fix incorrectly assign local checkpoint from max_seqno

e9a3932

Relates #27837

dnhatn added backport pending and removed backport pending labels Dec 20, 2017

dnhatn mentioned this pull request Dec 23, 2017

[CI] Global checkpoint assertion violated #27970

Closed

dnhatn mentioned this pull request Dec 23, 2017

Only sync translog when global checkpoint increased #27973

Closed

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

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


		final EngineConfig config = newEngineConfig(openMode);

		if (openMode == EngineConfig.OpenMode.OPEN_INDEX_AND_TRANSLOG) {

Conversation

ywelsch commented Dec 15, 2017

Uh oh!

bleskes commented Dec 15, 2017

Uh oh!

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

ywelsch commented Dec 18, 2017

Uh oh!

dnhatn commented Dec 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dnhatn commented Dec 20, 2017 •

edited

Loading