From 58a62abf740b593fc8ce743fb67319f621e7383c Mon Sep 17 00:00:00 2001 From: Nhat Nguyen Date: Thu, 25 Jan 2018 17:52:28 -0500 Subject: [PATCH] Add lower bound for translog flush threshold If the translog flush threshold is too small (eg. smaller than the translog header), we may repeatedly flush even there is no uncommitted operation because the shouldFlush condition can still be true after flushing. This is currently avoided by adding an extra guard against the uncommitted operations. However, this extra guard makes the shouldFlush complicated. This commit replaces that extra guard by a lower bound for translog flush threshold. We keep the lower bound small for convenience in testing. Relates #28350 Relates #23606 --- .../org/elasticsearch/index/IndexSettings.java | 15 +++++++++++---- .../index/engine/InternalEngine.java | 9 +-------- .../elasticsearch/index/translog/Translog.java | 1 + .../elasticsearch/index/shard/IndexShardIT.java | 2 +- 4 files changed, 14 insertions(+), 13 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/IndexSettings.java b/server/src/main/java/org/elasticsearch/index/IndexSettings.java index 5baca022a216a..093e110cb827b 100644 --- a/server/src/main/java/org/elasticsearch/index/IndexSettings.java +++ b/server/src/main/java/org/elasticsearch/index/IndexSettings.java @@ -183,8 +183,15 @@ public final class IndexSettings { Setting.timeSetting("index.refresh_interval", DEFAULT_REFRESH_INTERVAL, new TimeValue(-1, TimeUnit.MILLISECONDS), Property.Dynamic, Property.IndexScope); public static final Setting INDEX_TRANSLOG_FLUSH_THRESHOLD_SIZE_SETTING = - Setting.byteSizeSetting("index.translog.flush_threshold_size", new ByteSizeValue(512, ByteSizeUnit.MB), Property.Dynamic, - Property.IndexScope); + Setting.byteSizeSetting("index.translog.flush_threshold_size", new ByteSizeValue(512, ByteSizeUnit.MB), + /* + * An empty translog occupies 43 bytes on disk. If the flush threshold is below this, the flush thread + * can get stuck in an infinite loop as the shouldPeriodicallyFlush can still be true after flushing. + * However, small thresholds are useful for testing so we do not add a large lower bound here. + */ + new ByteSizeValue(Translog.DEFAULT_HEADER_SIZE_IN_BYTES + 1, ByteSizeUnit.BYTES), + new ByteSizeValue(Long.MAX_VALUE, ByteSizeUnit.BYTES), + Property.Dynamic, Property.IndexScope); /** * Controls how long translog files that are no longer needed for persistence reasons @@ -219,9 +226,9 @@ public final class IndexSettings { * generation threshold. However, small thresholds are useful for testing so we * do not add a large lower bound here. */ - new ByteSizeValue(64, ByteSizeUnit.BYTES), + new ByteSizeValue(Translog.DEFAULT_HEADER_SIZE_IN_BYTES + 1, ByteSizeUnit.BYTES), new ByteSizeValue(Long.MAX_VALUE, ByteSizeUnit.BYTES), - new Property[]{Property.Dynamic, Property.IndexScope}); + Property.Dynamic, Property.IndexScope); /** * Index setting to enable / disable deletes garbage collection. diff --git a/server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java b/server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java index c98b7763d1dbd..da21500dd1651 100644 --- a/server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java +++ b/server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java @@ -1477,14 +1477,7 @@ public boolean shouldPeriodicallyFlush() { * thus the IndexWriter#hasUncommittedChanges condition is not considered. */ final long uncommittedSizeOfNewCommit = translog.sizeOfGensAboveSeqNoInBytes(localCheckpointTracker.getCheckpoint() + 1); - /* - * If flushThreshold is too small, we may repeatedly flush even there is no uncommitted operation - * as #sizeOfGensAboveSeqNoInByte and #uncommittedSizeInBytes can return different values. - * An empty translog file has non-zero `uncommittedSize` (the translog header), and method #sizeOfGensAboveSeqNoInBytes can - * return 0 now(no translog gen contains ops above local checkpoint) but method #uncommittedSizeInBytes will return an actual - * non-zero value after rolling a new translog generation. This can be avoided by checking the actual uncommitted operations. - */ - return uncommittedSizeOfNewCommit < uncommittedSizeOfCurrentCommit && translog.uncommittedOperations() > 0; + return uncommittedSizeOfNewCommit < uncommittedSizeOfCurrentCommit; } @Override diff --git a/server/src/main/java/org/elasticsearch/index/translog/Translog.java b/server/src/main/java/org/elasticsearch/index/translog/Translog.java index 3cbc8fc530539..5bb36f90e6d44 100644 --- a/server/src/main/java/org/elasticsearch/index/translog/Translog.java +++ b/server/src/main/java/org/elasticsearch/index/translog/Translog.java @@ -108,6 +108,7 @@ public class Translog extends AbstractIndexShardComponent implements IndexShardC public static final String CHECKPOINT_FILE_NAME = "translog" + CHECKPOINT_SUFFIX; static final Pattern PARSE_STRICT_ID_PATTERN = Pattern.compile("^" + TRANSLOG_FILE_PREFIX + "(\\d+)(\\.tlog)$"); + public static final int DEFAULT_HEADER_SIZE_IN_BYTES = TranslogWriter.getHeaderLength(UUIDs.randomBase64UUID()); // the list of translog readers is guaranteed to be in order of translog generation private final List readers = new ArrayList<>(); diff --git a/server/src/test/java/org/elasticsearch/index/shard/IndexShardIT.java b/server/src/test/java/org/elasticsearch/index/shard/IndexShardIT.java index 601eb8e9b1d66..dbbf4bac3270d 100644 --- a/server/src/test/java/org/elasticsearch/index/shard/IndexShardIT.java +++ b/server/src/test/java/org/elasticsearch/index/shard/IndexShardIT.java @@ -350,7 +350,7 @@ public void testMaybeFlush() throws Exception { }); assertEquals(0, translog.uncommittedOperations()); translog.sync(); - long size = translog.uncommittedSizeInBytes(); + long size = Math.max(translog.uncommittedSizeInBytes(), Translog.DEFAULT_HEADER_SIZE_IN_BYTES + 1); logger.info("--> current translog size: [{}] num_ops [{}] generation [{}]", translog.uncommittedSizeInBytes(), translog.uncommittedOperations(), translog.getGeneration()); client().admin().indices().prepareUpdateSettings("test").setSettings(Settings.builder().put(