Add primary term to translog header by dnhatn · Pull Request #29227 · elastic/elasticsearch

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

    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

        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/Engine Anything around managing Lucene and the Translog in an open shard. labels Mar 23, 2018

dnhatn requested review from bleskes and s1monw 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

Conversation

dnhatn commented Mar 23, 2018

Uh oh!

elasticmachine commented Mar 23, 2018

Uh oh!

dnhatn commented Mar 25, 2018

Uh oh!

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

dnhatn commented Mar 26, 2018

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!

s1monw commented Apr 11, 2018

Uh oh!

s1monw commented Apr 11, 2018

Uh oh!

s1monw left a comment

Choose a reason for hiding this comment

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!

dnhatn commented Apr 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants