Add upper limit for scroll expiry #26448

jimczi · 2017-08-30T19:36:14Z

Add upper limit for scroll expiry

This change adds a dynamic cluster setting named search.max_keep_alive.
It is used as an upper limit for scroll expiry time in scroll queries and defaults to 1 day.
For throttling purpose reindex overrides the default scroll expiry time automatically and could be affected by this change which is why we use an insanely high default value at the moment (1 day).
This change also ensures that the existing setting search.default_keep_alive is always smaller than search.max_keep_alive.

Relates #11511
Fixes #23268

This change adds a dynamic cluster setting named `search.max_keep_alive`. It is used as an upper limit for scroll expiry time in scroll queries and defaults to 1 hour. This change also ensures that the existing setting `search.default_keep_alive` is always smaller than `search.max_keep_alive`. Relates elastic#11511

jimczi · 2017-08-31T12:18:22Z

Tests for _reindex fail with the default value of 1 day. We test that _reindex can be paused (by setting request_per_second to 0.0..1 and this creates very big expiry time for the internal scroll request.
I pushed a commit that bounds the maximum value for _reindex expiry time to 1h. This is just a workaround to make sure that _reindex does not wait too long between two scroll requests. @nik9000 I can open a separate PR for this but the workaround is pretty simple and not intrusive:
dc8238b

jpountz · 2017-09-01T08:46:04Z

core/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java

+     */
+    public synchronized <A, B> void addSettingsUpdateConsumer(Setting<A> a, Setting<B> b, BiConsumer<A, B> consumer) {
+        addSettingsUpdateConsumer(a, b, consumer, (i, j) -> {} );
+


remove empty line?

jpountz · 2017-09-01T08:48:53Z

core/src/test/java/org/elasticsearch/common/settings/SettingTests.java

+
+        public void validate(Integer a, Integer b) {
+            if (Integer.signum(a) != Integer.signum(b)) {
+                throw new IllegalArgumentException("boom");


nik9000

I expected some reindex integration tests to fail as well.....

nik9000 · 2017-09-05T16:01:15Z

core/src/main/java/org/elasticsearch/index/reindex/WorkerBulkByScrollTaskState.java

+    /**
+     * Maximum wait time allowed for throttling.
+     */
+    private static final long MAX_THROTTLE_WAIT_TIME =  TimeUnit.HOURS.toNanos(1);


I'd probably use a TimeValue instead. That way you never have to wonder "is this in nanos or millis?"

nik9000 · 2017-09-05T16:08:47Z

core/src/main/java/org/elasticsearch/index/reindex/WorkerBulkByScrollTaskState.java

@@ -189,7 +195,8 @@ public void delayPrepareBulkRequest(ThreadPool threadPool, TimeValue lastBatchSt

    public TimeValue throttleWaitTime(TimeValue lastBatchStartTime, TimeValue now, int lastBatchSize) {
        long earliestNextBatchStartTime = now.nanos() + (long) perfectlyThrottledBatchTime(lastBatchSize);
-        return timeValueNanos(max(0, earliestNextBatchStartTime - System.nanoTime()));
+        long waitTime = min(MAX_THROTTLE_WAIT_TIME, max(0, earliestNextBatchStartTime - System.nanoTime()));


I like limiting this here but think maybe we ought to also enforce it at request start and rethrottle time. We can work backwards from the batch size to reject requests that had a requests_per_second that is too small, I think.

I wonder if we should just do that in a followup.

I think it might be nice to use the actual value of the setting but that can be a "reindex only" followup thing, I think.

nik9000 · 2017-09-05T16:35:40Z

I expected some reindex integration tests to fail as well.....

Talked to Jim - I was wrong. No tests should fail because we never assert that we wait as long as we say we do. We have an integration test in reindex that starts the request very slow and then rethrottles it to speed it up but one hour is slow enough for it. If we modified reindex to fail user side requests that'd set the limit too long then the test would fail. We probably should do that as a separate change to reindex after this is merged just so no one is surprised about the sleep.

nik9000 · 2017-09-05T16:36:35Z

core/src/main/java/org/elasticsearch/index/reindex/WorkerBulkByScrollTaskState.java

@@ -189,7 +195,8 @@ public void delayPrepareBulkRequest(ThreadPool threadPool, TimeValue lastBatchSt

    public TimeValue throttleWaitTime(TimeValue lastBatchStartTime, TimeValue now, int lastBatchSize) {
        long earliestNextBatchStartTime = now.nanos() + (long) perfectlyThrottledBatchTime(lastBatchSize);
-        return timeValueNanos(max(0, earliestNextBatchStartTime - System.nanoTime()));
+        long waitTime = min(MAX_THROTTLE_WAIT_TIME, max(0, earliestNextBatchStartTime - System.nanoTime()));


I think it might be nice to use the actual value of the setting but that can be a "reindex only" followup thing, I think.

This change adds a dynamic cluster setting named `search.max_keep_alive`. It is used as an upper limit for scroll expiry time in scroll queries and defaults to 1 hour. This change also ensures that the existing setting `search.default_keep_alive` is always smaller than `search.max_keep_alive`. Relates #11511 * check style * add skip for bwc * iter * Add a maxium throttle wait time of 1h for reindex * review * remove empty line

jimczi added 3 commits August 30, 2017 21:27

check style

422ec54

add skip for bwc

2c722e0

jimczi added :Search/Search Search-related issues that do not fall into other categories >feature v6.1.0 v7.0.0 labels Aug 30, 2017

jimczi added 2 commits August 30, 2017 23:14

iter

1731e94

Add a maxium throttle wait time of 1h for reindex

dc8238b

jpountz approved these changes Sep 1, 2017

View reviewed changes

nik9000 reviewed Sep 5, 2017

View reviewed changes

nik9000 approved these changes Sep 5, 2017

View reviewed changes

jimczi added 2 commits September 6, 2017 09:06

review

b640c27

remove empty line

2166049

jimczi merged commit 0c799ee into elastic:master Sep 6, 2017

jimczi deleted the feature/scroll_max_keep_alive branch September 6, 2017 08:06

jimczi added a commit that referenced this pull request Sep 7, 2017

#26448: Fix rest test not cleaning the settings

b41c443

jimczi added a commit that referenced this pull request Sep 7, 2017

#26448: Fix rest test not cleaning the settings

ace3d1b

jimczi 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 upper limit for scroll expiry #26448

Add upper limit for scroll expiry #26448

jimczi commented Aug 30, 2017 •

edited

Loading

jimczi commented Aug 31, 2017

jpountz Sep 1, 2017

jpountz Sep 1, 2017

nik9000 left a comment

nik9000 Sep 5, 2017

nik9000 Sep 5, 2017

nik9000 Sep 5, 2017

nik9000 Sep 5, 2017

nik9000 commented Sep 5, 2017

nik9000 Sep 5, 2017

Add upper limit for scroll expiry #26448

Add upper limit for scroll expiry #26448

Conversation

jimczi commented Aug 30, 2017 • edited Loading

jimczi commented Aug 31, 2017

jpountz Sep 1, 2017

Choose a reason for hiding this comment

jpountz Sep 1, 2017

Choose a reason for hiding this comment

nik9000 left a comment

Choose a reason for hiding this comment

nik9000 Sep 5, 2017

Choose a reason for hiding this comment

nik9000 Sep 5, 2017

Choose a reason for hiding this comment

nik9000 Sep 5, 2017

Choose a reason for hiding this comment

nik9000 Sep 5, 2017

Choose a reason for hiding this comment

nik9000 commented Sep 5, 2017

nik9000 Sep 5, 2017

Choose a reason for hiding this comment

jimczi commented Aug 30, 2017 •

edited

Loading