Make setting index.translog.sync_interval be dynamic #37382

liketic · 2019-01-12T11:48:10Z

Currently, we cannot update index setting index.translog.sync_interval if index is open, because it's not dynamic which can be updated for closed index only.

Closes #32763

elasticmachine · 2019-01-14T09:18:19Z

Pinging @elastic/es-distributed

ywelsch

This PR has the same flaw as the attempt in #34104. Repeating my comment here:
While there's testing on the IndexSettings level, I don't think that the update logic actually takes effect on a live shard. The reason for this is that the fsyncTask (see IndexService) is currently only rescheduled when translog durability changes, not when the translog sync interval changes.

liketic · 2019-01-19T14:57:38Z

Thanks @ywelsch ! I pushed f5a6343 , could you please review again ?

ywelsch · 2019-01-21T09:55:35Z

@elasticmachine test this please

ywelsch

Thanks for the update @liketic. I think there's still a bug in this code, which means we should also look at beefing up the tests around setting both durability and the sync interval. I've added a comment to detail how to simplify the implementation. Let me know if you need more assistance on this.

ywelsch · 2019-01-21T10:12:20Z

server/src/main/java/org/elasticsearch/index/IndexService.java

-            if (durability != oldTranslogDurability) {
+            final TimeValue syncInterval = indexSettings.getTranslogSyncInterval();
+            if (syncInterval.equals(oldSyncInterval) == false) {
+                rescheduleFsyncTask(() -> new AsyncTranslogFSync(IndexService.this));


What if both durability and sync interval are changed in the same update? This update here ignores the change to durability. In particular, if durability is SYNC, this will now suddenly schedule a sync task when the sync interval is changed even though there's no such sync task supposed to run due to durability SYNC.

It might be better to restructure the sync task scheduling code to not look at oldTranslogDurability but just at whether we have a current task and whether we're supposed to have one, and if we have both, check that the intervals match (you can get the current tasks interval directly from the task (see refreshTask.getInterval() as an example a few lines above it), and then just do a plain rescheduleFsyncTask without parameters where it gets all it needs from the indexSettings (which have been updated).

Thanks @ywelsch , so are we going to do something like:

if (fsyncTask != null && durability == Translog.Durability.ASYNC && fsyncTask.getInterval().equals(indexSettings.getTranslogSyncInterval()) == false) { rescheduleFsyncTask(); } private void rescheduleFsyncTask() { try { if (fsyncTask != null) { fsyncTask.close(); } } finally { fsyncTask = indexSettings.getTranslogDurability() == Translog.Durability.REQUEST ? null : new AsyncTranslogFSync(this); } }

? I'm a little confused, what if the current fsyncTask is null or durability is changed from ASYNC to REQUEST, the rescheduleFsyncTask() will not be called. Or we can check if durability is changed first, like:

final Translog.Durability oldTranslogDurability = indexSettings.getTranslogDurability(); if (oldTranslogDurability != durability) { rescheduleFsyncTask(); } else if (fsyncTask != null && fsyncTask.getInterval().equals(indexSettings.getTranslogSyncInterval()) == false) { rescheduleFsyncTask(); } } } private void rescheduleFsyncTask() { try { if (fsyncTask != null) { fsyncTask.close(); } } finally { fsyncTask = indexSettings.getTranslogDurability() == Translog.Durability.REQUEST ? null : new AsyncTranslogFSync(this); } }

Could you help me? Thanks in advance.

How about the following:

diff --git a/server/src/main/java/org/elasticsearch/index/IndexService.java b/server/src/main/java/org/elasticsearch/index/IndexService.java index 604d27a1e70..da95ef3eff2 100644 --- a/server/src/main/java/org/elasticsearch/index/IndexService.java +++ b/server/src/main/java/org/elasticsearch/index/IndexService.java @@ -197,7 +197,7 @@ public class IndexService extends AbstractIndexComponent implements IndicesClust this.refreshTask = new AsyncRefreshTask(this); this.trimTranslogTask = new AsyncTrimTranslogTask(this); this.globalCheckpointTask = new AsyncGlobalCheckpointTask(this); - rescheduleFsyncTask(indexSettings.getTranslogDurability()); + updateFsyncTaskIfNecessary(); } public int numberOfShards() { @@ -636,8 +636,6 @@ public class IndexService extends AbstractIndexComponent implements IndicesClust @Override public synchronized void updateMetaData(final IndexMetaData currentIndexMetaData, final IndexMetaData newIndexMetaData) { - final Translog.Durability oldTranslogDurability = indexSettings.getTranslogDurability(); - final boolean updateIndexMetaData = indexSettings.updateIndexMetaData(newIndexMetaData); if (Assertions.ENABLED @@ -689,20 +687,23 @@ public class IndexService extends AbstractIndexComponent implements IndicesClust }); rescheduleRefreshTasks(); } - final Translog.Durability durability = indexSettings.getTranslogDurability(); - if (durability != oldTranslogDurability) { - rescheduleFsyncTask(durability); - } + updateFsyncTaskIfNecessary(); } } - private void rescheduleFsyncTask(Translog.Durability durability) { - try { - if (fsyncTask != null) { - fsyncTask.close(); + private void updateFsyncTaskIfNecessary() { + if (indexSettings.getTranslogDurability() == Translog.Durability.REQUEST) { + try { + if (fsyncTask != null) { + fsyncTask.close(); + } + } finally { + fsyncTask = null; } - } finally { - fsyncTask = durability == Translog.Durability.REQUEST ? null : new AsyncTranslogFSync(this); + } else if (fsyncTask == null) { + fsyncTask = new AsyncTranslogFSync(this); + } else { + fsyncTask.updateIfNeeded(); } } @@ -841,6 +842,13 @@ public class IndexService extends AbstractIndexComponent implements IndicesClust super(indexService, indexService.getIndexSettings().getTranslogSyncInterval()); } + void updateIfNeeded() { + final TimeValue newInterval = indexService.getIndexSettings().getTranslogSyncInterval(); + if (newInterval.equals(getInterval()) == false) { + setInterval(newInterval); + } + } + @Override protected String getThreadPool() { return ThreadPool.Names.FLUSH;

…ync_interval-dynamic

michaelbaamonde · 2019-02-28T21:40:38Z

@liketic Are you still interested in moving this forward?

liketic · 2019-03-01T00:46:31Z

@michaelbaamonde Yes, will update soon.

ywelsch · 2019-03-18T10:21:03Z

ping @liketic

…ync_interval-dynamic

liketic · 2019-03-19T01:09:11Z

@ywelsch please review again.

ywelsch

LGTM. Thank you @liketic!

ywelsch · 2019-03-19T13:11:09Z

@elasticmachine retest this please

ywelsch · 2019-03-19T13:45:54Z

@elasticmachine retest this please

ywelsch · 2019-03-20T09:14:53Z

@elasticmachine retest this please

ywelsch · 2019-03-20T09:48:09Z

CI run failed for unrelated reasons:

Could not determine the dependencies of task ':distribution:extractWindowsJdk'.
[7.1.0] > Could not resolve all files for configuration ':distribution:jdk_windows'.
[7.1.0] > Could not download windows.zip (jdk:windows:12)
[7.1.0] > Could not get resource 'https://download.java.net/java/GA/jdk12/33/GPL/openjdk-12_windows-x64_bin.zip'.
[7.1.0] > Premature end of Content-Length delimited message body (expected: 196405895; received: 2097152
[7.1.0]

@elasticmachine retest this please

ywelsch · 2019-03-20T12:18:00Z

another unrelated test failure:

ERROR 0.17s J2 | InternalEngineTests.testDeleteWithFatalError <<< FAILURES!
Throwable #1: java.io.IOException: could not remove the following files (in the order of attempts):
/var/lib/jenkins/workspace/elastic+elasticsearch+pull-request-1/server/build/testrun/unitTest/J2/temp/org.elasticsearch.index.engine.InternalEngineTests_D5B77D5B191D2491-001/translog-primary-001/translog-2.tlog: java.io.IOException: access denied: /var/lib/jenkins/workspace/elastic+elasticsearch+pull-request-1/server/build/testrun/unitTest/J2/temp/org.elasticsearch.index.engine.InternalEngineTests_D5B77D5B191D2491-001/translog-primary-001/translog-2.tlog
/var/lib/jenkins/workspace/elastic+elasticsearch+pull-request-1/server/build/testrun/unitTest/J2/temp/org.elasticsearch.index.engine.InternalEngineTests_D5B77D5B191D2491-001/translog-primary-001/translog-1.tlog: java.io.IOException: access denied: /var/lib/jenkins/workspace/elastic+elasticsearch+pull-request-1/server/build/testrun/unitTest/J2/temp/org.elasticsearch.index.engine.InternalEngineTests_D5B77D5B191D2491-001/translog-primary-001/translog-1.tlog
/var/lib/jenkins/workspace/elastic+elasticsearch+pull-request-1/server/build/testrun/unitTest/J2/temp/org.elasticsearch.index.engine.InternalEngineTests_D5B77D5B191D2491-001/translog-primary-001: java.nio.file.DirectoryNotEmptyException: /var/lib/jenkins/workspace/elastic+elasticsearch+pull-request-1/server/build/testrun/unitTest/J2/temp/org.elasticsearch.index.engine.InternalEngineTests_D5B77D5B191D2491-001/translog-primary-001
at __randomizedtesting.SeedInfo.seed([D5B77D5B191D2491:39307CC9B117DA9]:0)
at org.elasticsearch.core.internal.io.IOUtils.rm(IOUtils.java:196)
at org.elasticsearch.index.translog.Translog.createEmptyTranslog(Translog.java:1865)
at org.elasticsearch.index.translog.Translog.createEmptyTranslog(Translog.java:1860)
at org.elasticsearch.index.engine.EngineTestCase.createEngine(EngineTestCase.java:511)
at org.elasticsearch.index.engine.InternalEngineTests.testDeleteWithFatalError(InternalEngineTests.java:3318)
at java.lang.Thread.run(Thread.java:748)

/cc: @dnhatn

@elasticmachine run elasticsearch-ci/1

Currently, we cannot update index setting index.translog.sync_interval if index is open, because it's not dynamic which can be updated for closed index only. Closes #32763

Currently, we cannot update index setting index.translog.sync_interval if index is open, because it's not dynamic which can be updated for closed index only. Closes elastic#32763

Make setting index.translog.sync_interval be dynamic

443f31b

colings86 added the :Distributed/Engine Anything around managing Lucene and the Translog in an open shard. label Jan 14, 2019

ywelsch self-requested a review January 14, 2019 10:33

ywelsch suggested changes Jan 14, 2019

View reviewed changes

ywelsch mentioned this pull request Jan 14, 2019

Documentation claims "index.translog.sync_interval" is dynamic, but it can not be set dynamically #32763

Closed

Make sync interval updates work

f5a6343

ywelsch self-requested a review January 21, 2019 09:33

ywelsch suggested changes Jan 21, 2019

View reviewed changes

Merge branch 'master' of github.com:elastic/elasticsearch into make-s…

964acab

…ync_interval-dynamic

liketic added 2 commits March 18, 2019 22:41

Merge branch 'master' of github.com:elastic/elasticsearch into make-s…

8f0ae18

…ync_interval-dynamic

Address comments

018da3d

ywelsch self-requested a review March 19, 2019 09:41

ywelsch approved these changes Mar 19, 2019

View reviewed changes

ywelsch added 2 commits March 19, 2019 14:44

checkstyle fix

79c1ef1

Merge remote-tracking branch 'elastic/master' into pr/37382

a159d22

ywelsch added >enhancement v8.0.0 v7.2.0 labels Mar 19, 2019

ywelsch added 2 commits March 20, 2019 10:13

fix test

9eac473

Merge remote-tracking branch 'elastic/master' into pr/37382

a8d466d

ywelsch merged commit 3c352a8 into elastic:master Mar 20, 2019

mkleen mentioned this pull request Oct 8, 2020

Backport of IndexServiceTest crate/crate#10628

Merged

5 tasks

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make setting index.translog.sync_interval be dynamic #37382

Make setting index.translog.sync_interval be dynamic #37382

liketic commented Jan 12, 2019

elasticmachine commented Jan 14, 2019

ywelsch left a comment

liketic commented Jan 19, 2019

ywelsch commented Jan 21, 2019

ywelsch left a comment

ywelsch Jan 21, 2019

liketic Jan 26, 2019

ywelsch Jan 29, 2019

michaelbaamonde commented Feb 28, 2019

liketic commented Mar 1, 2019

ywelsch commented Mar 18, 2019

liketic commented Mar 19, 2019

ywelsch left a comment

ywelsch commented Mar 19, 2019

ywelsch commented Mar 19, 2019

ywelsch commented Mar 20, 2019

ywelsch commented Mar 20, 2019

ywelsch commented Mar 20, 2019

Make setting index.translog.sync_interval be dynamic #37382

Make setting index.translog.sync_interval be dynamic #37382

Conversation

liketic commented Jan 12, 2019

elasticmachine commented Jan 14, 2019

ywelsch left a comment

Choose a reason for hiding this comment

liketic commented Jan 19, 2019

ywelsch commented Jan 21, 2019

ywelsch left a comment

Choose a reason for hiding this comment

ywelsch Jan 21, 2019

Choose a reason for hiding this comment

liketic Jan 26, 2019

Choose a reason for hiding this comment

ywelsch Jan 29, 2019

Choose a reason for hiding this comment

michaelbaamonde commented Feb 28, 2019

liketic commented Mar 1, 2019

ywelsch commented Mar 18, 2019

liketic commented Mar 19, 2019

ywelsch left a comment

Choose a reason for hiding this comment

ywelsch commented Mar 19, 2019

ywelsch commented Mar 19, 2019

ywelsch commented Mar 20, 2019

ywelsch commented Mar 20, 2019

ywelsch commented Mar 20, 2019