Add primary term to translog header #29227

dnhatn · 2018-03-23T19:49:03Z

This commit adds the current primary term to the header of the current translog file. Having a term in a translog header is a prerequisite step that allows us to trim translog operations given the max valid seq# for that term.

This commit also updates tests to conform the primary term invariant which guarantees that all translog operations in a translog file have its terms at most the term stored in the translog header.

This commit adds the current primary term to the header of the current translog file. Having a term in a translog file allows us to trim translog operations in that file given the max valid seq# for that term. This commit also updates tests to conform the primary term invariant which guarantees that all translog operations in a translog file have its terms at most the term stored in the translog header.

elasticmachine · 2018-03-23T19:49:05Z

Pinging @elastic/es-distributed

dnhatn · 2018-03-25T23:42:49Z

@bleskes Please let me know if I need to split this into two separate PRs.

s1monw

left some initial comments. looks good

s1monw · 2018-03-26T10:01:33Z

server/src/main/java/org/elasticsearch/index/translog/TranslogHeader.java

+        // Write primary term
+        out.writeLong(primaryTerm);
+        // Checksum header
+        out.writeInt((int) out.getChecksum());


I think you should call out.flush() here to ensure everything is written to the channel.

s1monw · 2018-03-26T10:01:55Z

server/src/main/java/org/elasticsearch/index/translog/TranslogHeader.java

+
+    /**
+     * Writes this header with the latest format into the file channel
+     */


check if you need to put a SuppressWarning here since we don't close the channel.

Yes. I added a SuppressWarning for both Eclipse and Intellij.

s1monw · 2018-03-26T10:04:42Z

server/src/main/java/org/elasticsearch/index/translog/TranslogReader.java

-        try {
-            InputStreamStreamInput headerStream = new InputStreamStreamInput(java.nio.channels.Channels.newInputStream(channel),
-                channel.size()); // don't close
-            // Lucene's CodecUtil writes a magic number of 0x3FD76C17 with the


this PR removes this LUCENE_CODEC_HEADER_BYTE check etc. since we don't support it, correct. Maybe we should keep it and fail with a descriptive error message instead?

I've restored this.

# Conflicts: # server/src/test/java/org/elasticsearch/index/engine/EngineDiskUtilsTests.java # server/src/test/java/org/elasticsearch/index/shard/RefreshListenersTests.java

dnhatn · 2018-03-26T18:20:20Z

@s1monw I've addressed your comments. Would you please take another look? Thank you.

bleskes

Looks good. I left some nits.

bleskes · 2018-03-27T19:00:50Z

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

@@ -140,10 +141,11 @@
     */
    public Translog(
        final TranslogConfig config, final String translogUUID, TranslogDeletionPolicy deletionPolicy,
-        final LongSupplier globalCheckpointSupplier) throws IOException {
+        final LongSupplier globalCheckpointSupplier, final LongSupplier primaryTermSupplier) throws IOException {


can you add java docs for the term supplier ? also describe what the term means here - i.e., it is sampled when a generation is rolled and the translog will reject operations with a higher term until rolled again.

bleskes · 2018-03-27T19:15:35Z

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

@@ -1723,7 +1728,7 @@ static String createEmptyTranslog(Path location, long initialGlobalCheckpoint, S
        final String translogUUID = UUIDs.randomBase64UUID();
        TranslogWriter writer = TranslogWriter.create(shardId, translogUUID, 1, location.resolve(getFilename(1)), channelFactory,
            new ByteSizeValue(10), 1, initialGlobalCheckpoint,
-            () -> { throw new UnsupportedOperationException(); }, () -> { throw new UnsupportedOperationException(); }
+            () -> { throw new UnsupportedOperationException(); }, () -> { throw new UnsupportedOperationException(); }, 0L


shall we make people pass the term to this method? If it's difficult, it's not a big deal but I feel it will be good to have the right term on the file.

I pushed ad5a212

bleskes · 2018-03-27T19:30:37Z

server/src/main/java/org/elasticsearch/index/translog/TranslogHeader.java

+    private final long primaryTerm;
+    private final int headerSizeInBytes;
+
+    TranslogHeader(String translogUUID, long primaryTerm) {


can we add java docs about what the uuid and term mean? i.e., what do they enforce?

bleskes · 2018-03-27T19:31:33Z

server/src/main/java/org/elasticsearch/index/translog/TranslogHeader.java

+        return headerSizeInBytes;
+    }
+
+    static int defaultSizeInBytes(String translogUUID) {


nit: this is used in the constructor so it's not really default, maybe just headerSizeInBytes?

bleskes · 2018-03-27T19:41:31Z

server/src/main/java/org/elasticsearch/index/translog/TranslogHeader.java

+        // 0x00 => version 0 of the translog
+        final byte b1 = Channels.readFromFileChannel(channel, 0, 1)[0];
+        if (b1 == 0x3f) { // LUCENE_CODEC_HEADER_BYTE
+            throw new TranslogCorruptedException("translog looks like version 1 or later, but has corrupted header. path:" + path);


can you clarify what you mean with version 1 or later?

It means that the translog was created in ES 1.4 or later but corrupted. Moved this from

elasticsearch/server/src/main/java/org/elasticsearch/index/translog/TranslogReader.java

Line 110 in 4ef3de4

throw new TranslogCorruptedException("translog looks like version 1 or later, but has corrupted header. path:" + path);

bleskes · 2018-03-27T19:50:05Z

server/src/main/java/org/elasticsearch/index/translog/TranslogHeader.java

+            assert primaryTerm >= 0 : "Primary term must be non-negative [" + primaryTerm + "]; translog path [" + path + "]";
+        } else {
+            assert version == VERSION_CHECKPOINTS : "Unknown header version [" + version + "]";
+            primaryTerm = UNKNOWN_PRIMARY_TERM;


UNKOWN_PRIMARY_TERM is -1. I wonder if we should 0 as a default here. Primary terms are always non negative.

Yes, we can use 0L as the default. I pushed d50d7ed

bleskes · 2018-03-27T19:50:25Z

server/src/main/java/org/elasticsearch/index/translog/TranslogHeader.java

+        }
+        // Verify the checksum
+        if (version >= VERSION_PRIMARY_TERM) {
+            Translog.verifyChecksum(in);


s1monw · 2018-04-11T14:48:29Z

@dnhatn this has many many unrelated changes I think some merge was messed up?

s1monw · 2018-04-11T14:49:50Z

@dnhatn nervermind I think github messed something up when I check what changed

# Conflicts: # server/src/main/java/org/elasticsearch/index/translog/Translog.java # server/src/test/java/org/elasticsearch/action/resync/ResyncReplicationRequestTests.java # server/src/test/java/org/elasticsearch/index/IndexModuleTests.java # server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java # server/src/test/java/org/elasticsearch/index/shard/IndexingOperationListenerTests.java # server/src/test/java/org/elasticsearch/index/translog/TranslogTests.java # server/src/test/java/org/elasticsearch/indices/recovery/RecoverySourceHandlerTests.java

s1monw

LGTM

bleskes

LGTM. Thanks @dnhatn

bleskes · 2018-04-12T15:55:44Z

server/src/main/java/org/elasticsearch/index/translog/TranslogHeader.java

+        // 0x00 => version 0 of the translog
+        final byte b1 = Channels.readFromFileChannel(channel, 0, 1)[0];
+        if (b1 == 0x3f) { // LUCENE_CODEC_HEADER_BYTE
+            throw new TranslogCorruptedException("translog looks like version 1 or later, but has corrupted header. path:" + path);


bleskes · 2018-04-12T15:56:51Z

server/src/main/java/org/elasticsearch/index/translog/TruncateTranslogCommand.java

-            TranslogWriter.writeHeader(out, translogRef);
-            fc.force(true);
+        try (FileChannel fc = FileChannel.open(filename, StandardOpenOption.WRITE, StandardOpenOption.CREATE_NEW)) {
+            TranslogHeader header = new TranslogHeader(translogUUID, 0L);


Shall we use the DEFAULT_PRIMARY_TERM?

dnhatn · 2018-04-12T17:57:53Z

Thanks @s1monw and @bleskes for reviewing.

This change adds the current primary term to the header of the current translog file. Having a term in a translog header is a prerequisite step that allows us to trim translog operations given the max valid seq# for that term. This commit also updates tests to conform the primary term invariant which guarantees that all translog operations in a translog file have its terms at most the term stored in the translog header.

* master: Enable skipping fetching latest for BWC builds (elastic#29497) Add remote cluster client (elastic#29495) Ensure flush happens on shard idle Adds SpanGapQueryBuilder in the query DSL (elastic#28636) Control max size and count of warning headers (elastic#28427) Make index APIs work without types. (elastic#29479) Deprecate filtering on `_type`. (elastic#29468) Fix auto-generated ID example format (elastic#29461) Fix typo in max number of threads check docs (elastic#29469) Add primary term to translog header (elastic#29227) Add a helper method to get a random java.util.TimeZone (elastic#29487) Move TimeValue into elasticsearch-core project (elastic#29486)

* es/master: Add remote cluster client (#29495) Ensure flush happens on shard idle Adds SpanGapQueryBuilder in the query DSL (#28636) Control max size and count of warning headers (#28427) Make index APIs work without types. (#29479) Deprecate filtering on `_type`. (#29468) Fix auto-generated ID example format (#29461) Fix typo in max number of threads check docs (#29469) Add primary term to translog header (#29227) Add a helper method to get a random java.util.TimeZone (#29487) Move TimeValue into elasticsearch-core project (#29486) Fix NPE in InternalGeoCentroidTests#testReduceRandom (#29481) Build: introduce keystoreFile for cluster config (#29491) test: Index more docs, so that it is less likely the search request does not time out.

* es/6.x: Enable skipping fetching latest for BWC builds (#29497) Add remote cluster client (#29495) Ensure flush happens on shard idle Adds SpanGapQueryBuilder in the query DSL (#28636) Fix auto-generated ID example format (#29461) Fix typo in max number of threads check docs (#29469) Add primary term to translog header (#29227) Add a helper method to get a random java.util.TimeZone (#29487) Move TimeValue into elasticsearch-core project (#29486) Fix NPE in InternalGeoCentroidTests#testReduceRandom (#29481) Build: introduce keystoreFile for cluster config (#29491) test: Index more docs, so that it is less likely the search request does not time out.

dnhatn added >enhancement v7.0.0 v6.3.0 :Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. labels Mar 23, 2018

dnhatn requested review from s1monw and bleskes March 23, 2018 19:49

dnhatn added 2 commits March 23, 2018 21:13

waits for primary promotion

fe025b7

Read header when reading global checkpoint

8aac0a1

s1monw suggested changes Mar 26, 2018

View reviewed changes

dnhatn added 3 commits March 26, 2018 11:25

Merge branch 'master' into add-term-tlog-header

010ceca

# Conflicts: # server/src/test/java/org/elasticsearch/index/engine/EngineDiskUtilsTests.java # server/src/test/java/org/elasticsearch/index/shard/RefreshListenersTests.java

Restore old versions check

46d0c4f

add SuppressWarnings for eclipse and intellij

0d914a0

comments

4999d06

bleskes suggested changes Mar 27, 2018

View reviewed changes

dnhatn added 7 commits March 28, 2018 11:48

Merge branch 'master' into add-term-tlog-header

6325841

Merge branch 'master' into add-term-tlog-header

4f1da3c

wip - add doc

374eba8

javadocs

4b7c371

use 0L for unknown term

d50d7ed

bootstrap primary term for an empty translog

ad5a212

fix bootstrap primary term test

6ea0779

dnhatn requested a review from s1monw April 11, 2018 14:07

s1monw approved these changes Apr 11, 2018

View reviewed changes

style check

fc8bc06

dnhatn requested a review from bleskes April 11, 2018 15:34

bleskes approved these changes Apr 12, 2018

View reviewed changes

dnhatn added 2 commits April 12, 2018 12:03

Merge branch 'master' into add-term-tlog-header

3a49328

Use the primary term constant instead of 0L

b61d459

dnhatn merged commit f96e00b into elastic:master Apr 12, 2018

dnhatn deleted the add-term-tlog-header branch April 12, 2018 17:58

dnhatn added the backport pending label Apr 12, 2018

dnhatn removed the backport pending label Apr 13, 2018

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

DaveCTurner mentioned this pull request May 30, 2019

Remove support for VERSION_CHECKPOINTS translogs #42699

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add primary term to translog header #29227

Add primary term to translog header #29227

dnhatn commented Mar 23, 2018

elasticmachine commented Mar 23, 2018

dnhatn commented Mar 25, 2018

s1monw left a comment

s1monw Mar 26, 2018

dnhatn Mar 26, 2018

s1monw Mar 26, 2018

dnhatn Mar 26, 2018

s1monw Mar 26, 2018

dnhatn Mar 26, 2018

dnhatn commented Mar 26, 2018

bleskes left a comment

bleskes Mar 27, 2018

bleskes Mar 27, 2018

dnhatn Mar 28, 2018

bleskes Mar 27, 2018

bleskes Mar 27, 2018

bleskes Mar 27, 2018

dnhatn Mar 28, 2018

bleskes Apr 12, 2018

bleskes Mar 27, 2018

dnhatn Mar 28, 2018

bleskes Mar 27, 2018

s1monw commented Apr 11, 2018

s1monw commented Apr 11, 2018

s1monw left a comment

bleskes left a comment

bleskes Apr 12, 2018

bleskes Apr 12, 2018

dnhatn commented Apr 12, 2018

Add primary term to translog header #29227

Add primary term to translog header #29227

Conversation

dnhatn commented Mar 23, 2018

elasticmachine commented Mar 23, 2018

dnhatn commented Mar 25, 2018

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

dnhatn commented Mar 26, 2018

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

s1monw commented Apr 11, 2018

s1monw commented Apr 11, 2018

s1monw left a comment

Choose a reason for hiding this comment

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

dnhatn commented Apr 12, 2018