Add translog files age to Translog Stats (#28613) #28613

justinwyer · 2018-02-10T14:14:55Z

#28189 Implemented earliest_generation_last_modified_age as a translog stats item. This uses the last modified time of the earliest generation.

…translog stats item. This uses the last modified time of the earliest generation.

elasticmachine · 2018-02-10T14:14:57Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

elasticmachine · 2018-02-10T14:14:57Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

bleskes

Thx @justinwyer . Looking good. I left some comments.

bleskes · 2018-02-11T08:09:31Z

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

@@ -444,6 +450,20 @@ public long sizeOfGensAboveSeqNoInBytes(long minSeqNo) {
        }
    }

+    private long earliestGenerationLastModifiedAge(long minGeneration) {


I think we can reduce complexity here as we don't need min generation control. Can we remove the parameter please?

maybe make this method static, give it a list/stream of readers and the current time as parameters? this would allow for proper testing with mocks and fixed time.

A static method for testing is a good idea. If we made it static we would still need to wrap it in a non static method to include:

try (ReleasableLock ignored = readLock.acquire()) { ensureOpen(); ... }

bleskes · 2018-02-11T08:14:15Z

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

@@ -66,6 +72,9 @@ public void add(TranslogStats translogStats) {
        this.translogSizeInBytes += translogStats.translogSizeInBytes;
        this.uncommittedOperations += translogStats.uncommittedOperations;
        this.uncommittedSizeInBytes += translogStats.uncommittedSizeInBytes;
+        this.earliestGenerationLastModifiedAge =


can we use Math.min()?

bleskes · 2018-02-11T08:20:36Z

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

+            ensureOpen();
+            Optional<? extends BaseTranslogReader> earliestGeneration = Stream.concat(readers.stream(), Stream.of(current))
+                .filter(r -> r.getGeneration() >= minGeneration)
+                .min(Comparator.comparingLong(BaseTranslogReader::getGeneration));


can we maybe map this to a stream of longs via getLastModifiedTime and collect using Collectors.minBy(Comparator.comparing(BaseTranslogReader:: getLastModifiedTime)) or something similar? also, can we also take the writer into account for completeness?

To be clear - I think we want to know how the oldest file is, regardless of the generations. It will always be the oldest generation and the first in the reader list, but I don't think we want to rely on it. Part of the role of the stats is to validate things are correct.

Sure but we can't use BaseTranslogReader:: getLastModifiedTime as the method throws a checked exception.

Does Stream.concat(readers.stream(), Stream.of(current)) not include the writer?

Sure but we can't use BaseTranslogReader:: getLastModifiedTime as the method throws a checked exception.

Fair enough. No streams for us - we need to do it the old fashion way :D

Does Stream.concat(readers.stream(), Stream.of(current)) not include the writer?

Yes. Current is a TranslogWriter.

I've settled on the following WDYT?

long earliestLastModifiedAge() { try (ReleasableLock ignored = readLock.acquire()) { ensureOpen(); return findEarliestLastModifiedAge(System.currentTimeMillis(), Stream.concat(readers.stream(), Stream.of(current))); } catch (ElasticsearchException e) { throw new TranslogException(shardId, e.getDetailedMessage(), e); } } /** * Returns the age of the oldest entry in the translog files in seconds */ static long findEarliestLastModifiedAge(long currentTime, Stream<BaseTranslogReader> baseTranslogReaders) { return currentTime - baseTranslogReaders.mapToLong(Translog::uncheckedGetLastModifiedTime).min().orElse(currentTime); } private static long uncheckedGetLastModifiedTime(BaseTranslogReader r) { try { return r.getLastModifiedTime(); } catch (IOException e) { throw new ElasticsearchException("Unable to get the earliest last modified time for the transaction log"); } }

On the testing side I've added:

public void testFindEarliestLastModifiedAge() throws IOException { long fixedTime = System.currentTimeMillis(); long earlyTime = fixedTime - 10000; BaseTranslogReader early = mock(BaseTranslogReader.class); stub(early.getLastModifiedTime()).toReturn(earlyTime); long earlierTime = fixedTime - 20000; BaseTranslogReader earlier = mock(BaseTranslogReader.class); stub(earlier.getLastModifiedTime()).toReturn(earlierTime); long earliestTime = fixedTime - 30000; BaseTranslogReader earliest = mock(BaseTranslogReader.class); stub(earliest.getLastModifiedTime()).toReturn(earliestTime); Stream<BaseTranslogReader> readers = Stream.of(early, earlier, earliest); assertThat(Translog.findEarliestLastModifiedAge(fixedTime, readers), equalTo(30000L)); }

./gradlew check running now, feel free to comment soonest :D

I would rather not have the uncheckedGetLastModifiedTime method. Instead just do old school iteration on the reader list, keeping the min of the last modified time. Then do a one off extra min with the writer's value.

Re the test - I would prefer if you make an array of random positive long values, map readers to them and make sure the method returns the right value when compared to a calculation that is made directly on the array.

bleskes · 2018-02-11T08:21:03Z

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

+                .filter(r -> r.getGeneration() >= minGeneration)
+                .min(Comparator.comparingLong(BaseTranslogReader::getGeneration));
+            try {
+                return earliestGeneration.isPresent() ? earliestGeneration.get().getLastModifiedTime() : 0L;


the method indicates we return an age by this returns last modified time?

bleskes · 2018-02-14T08:38:17Z

@justinwyer can you ping me when this is ready for another cycle? I saw you pushed something but I don't want to look until you feel it's ready.

justinwyer · 2018-02-14T08:53:28Z

@bleskes I hope I covered all the concerns, you can take a look.

…o 28189

bleskes

Thanks. This is getting into shape. I left some minor comments. it will be great to have a rest test too, to make share the xcontent rending includes this info

bleskes · 2018-02-16T07:54:01Z

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

+        for (BaseTranslogReader r : readers) {
+            earliestTime = Math.min(r.getLastModifiedTime(), earliestTime);
+        }
+        return currentTime - Math.min(earliestTime, writer.getLastModifiedTime());


Can we add a Math.max(0, currentTime - Math.min()) ? we rely on this being non negative, but time may go back and the FS may have other quirks.

bleskes · 2018-02-16T07:58:23Z

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

@@ -356,6 +358,25 @@ protected TranslogStats stats() throws IOException {
        return stats;
    }

+    public void testFindEarliestLastModifiedAge() throws IOException {
+        long fixedTime = System.currentTimeMillis();
+        long[] periods = new long[10];


can we sometime test the case where have no readers?

bleskes · 2018-02-16T07:58:56Z

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

@@ -356,6 +358,25 @@ protected TranslogStats stats() throws IOException {
        return stats;
    }

+    public void testFindEarliestLastModifiedAge() throws IOException {
+        long fixedTime = System.currentTimeMillis();


it will be good to use randomized numbers here. It will be easier to reproduce failures and it will also test time go back.

justinwyer · 2018-02-16T09:10:57Z

@bleskes just to confirm when you say a rest test is adding checks to rest-api-spec/test/indices.stats/20_translog.yml what you mean or are there other places as well?

bleskes · 2018-02-16T10:00:22Z

Yes. That yml file is the right place. Sorry for not being explicit.

…

On 16 Feb 2018, at 10:11 AM, Justin Wyer ***@***.***> wrote: @bleskes just to confirm when you say a rest test is adding checks to rest-api-spec/test/indices.stats/20_translog.yml what you mean or are there other places as well? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

justinwyer · 2018-02-16T11:58:55Z

@bleskes you can take another look 😄

bleskes

LGTM. I left one little ask. I'll push this once it's done.

Thanks @justinwyer !

bleskes · 2018-02-16T12:25:04Z

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

-        long fixedTime = System.currentTimeMillis();
-        long[] periods = new long[10];
-        for (int i = 0; i < 9; i++) {
+        final int numberOfReaders = scaledRandomIntBetween(1, 10);


Can you make this go down to 0?

I explicitly test the 0 readers case in that test as well as a random number of readers between 1 and 10 I can do the change of you prefer however.

oh I see. I missed it.

bleskes · 2018-02-16T13:16:52Z

@elasticmachine please test this.

Expose the age of translog files in the translog stats. This is useful to reason about your translog retention policy. Closes #28189

Relates #28613

We added a rest test for the translog last modified age without a version check. This causes BWC failed because the stats are not available in the old versions. Relates #28613

…and add properties we weren't mapping

…arch#28613… (#3358) This commit adds additional translog properties as per elastic/elasticsearch#28613 and add properties we weren't mapping

…arch#28613… (#3358) This commit adds additional translog properties as per elastic/elasticsearch#28613 and add properties we weren't mapping (cherry picked from commit 7ff63ab)

elastic#28189 Implemented earliest_generation_last_modified_age as a …

948614e

…translog stats item. This uses the last modified time of the earliest generation.

bleskes suggested changes Feb 11, 2018

View reviewed changes

changes per review

056710a

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

e3586d1

…o 28189

bleskes suggested changes Feb 16, 2018

View reviewed changes

changes per second review

4066b42

bleskes approved these changes Feb 16, 2018

View reviewed changes

bleskes changed the title ~~#28189 translog age in stats~~ Add translog files age to Translog Stats (#28613) Feb 16, 2018

bleskes merged commit 5aeb479 into elastic:master Feb 16, 2018

bleskes added :Data Management/Stats Statistics tracking and retrieval APIs :Distributed/Engine Anything around managing Lucene and the Translog in an open shard. >enhancement v7.0.0 v6.3.0 backport pending labels Feb 16, 2018

dnhatn pushed a commit that referenced this pull request Feb 16, 2018

Add translog file age to Translog Stats (#28613)

e0e4c73

Expose the age of translog files in the translog stats. This is useful to reason about your translog retention policy. Closes #28189

dnhatn added a commit that referenced this pull request Feb 16, 2018

Backported the translog files age stats to v6.3.0

7d3e7ef

Relates #28613

dnhatn added a commit that referenced this pull request Feb 16, 2018

Backported the translog files age stats to v6.3.0

2f01129

Relates #28613

dnhatn removed the backport pending label Feb 16, 2018

russcam mentioned this pull request Jul 9, 2018

Add Translog files age to TranslogStats elastic/elasticsearch-net#3330

Closed

Mpdreamz added a commit to elastic/elasticsearch-net that referenced this pull request Aug 12, 2018

add additional translog properties as per elastic/elasticsearch#28613 …

c72508d

…and add properties we weren't mapping

Mpdreamz mentioned this pull request Sep 10, 2018

6.3.0 release elastic/elasticsearch-net#3386

Closed

45 tasks

jpountz removed the :Distributed/Engine Anything around managing Lucene and the Translog in an open shard. label Jan 29, 2019

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add translog files age to Translog Stats (#28613) #28613

Add translog files age to Translog Stats (#28613) #28613

justinwyer commented Feb 10, 2018

elasticmachine commented Feb 10, 2018

elasticmachine commented Feb 10, 2018

bleskes left a comment

bleskes Feb 11, 2018

bleskes Feb 11, 2018

justinwyer Feb 12, 2018 •

edited

bleskes Feb 12, 2018

bleskes Feb 11, 2018

bleskes Feb 11, 2018

bleskes Feb 11, 2018

justinwyer Feb 12, 2018

bleskes Feb 12, 2018

justinwyer Feb 12, 2018 •

edited

bleskes Feb 12, 2018

bleskes Feb 11, 2018

justinwyer Feb 12, 2018

bleskes commented Feb 14, 2018

justinwyer commented Feb 14, 2018

bleskes left a comment

bleskes Feb 16, 2018

bleskes Feb 16, 2018

bleskes Feb 16, 2018

justinwyer commented Feb 16, 2018

bleskes commented Feb 16, 2018 via email

justinwyer commented Feb 16, 2018

bleskes left a comment

bleskes Feb 16, 2018

justinwyer Feb 16, 2018

bleskes Feb 16, 2018

bleskes commented Feb 16, 2018

Add translog files age to Translog Stats (#28613) #28613

Add translog files age to Translog Stats (#28613) #28613

Conversation

justinwyer commented Feb 10, 2018

elasticmachine commented Feb 10, 2018

elasticmachine commented Feb 10, 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

justinwyer Feb 12, 2018 • edited

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

justinwyer Feb 12, 2018 • edited

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

bleskes commented Feb 14, 2018

justinwyer commented Feb 14, 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

justinwyer commented Feb 16, 2018

bleskes commented Feb 16, 2018 via email

justinwyer commented Feb 16, 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

bleskes commented Feb 16, 2018

justinwyer Feb 12, 2018 •

edited

justinwyer Feb 12, 2018 •

edited