Implement rejections in `WriteMemoryLimits` #58885

Tim-Brooks · 2020-07-02T00:51:30Z

This commit adds rejections when the indexing memory limits are
exceeded for primary or coordinating operations. The amount of bytes
allow for indexing is controlled by a new setting
indices.write.limit.

Relates #59263

elasticmachine · 2020-07-02T00:51:32Z

Pinging @elastic/es-distributed (:Distributed/CRUD)

Tim-Brooks · 2020-07-02T02:10:15Z

@ywelsch - I set the rejection threshold to 30% because a decent amount of tests index large enough bulk requests to trip it if it is 10% (reindex, searchable snapshot, rollover, etc). Once we decide on a number, we might need to artificially inflate it in the test configurations due to the small heap sizes.

ywelsch

I've left some comments. The main effort of this PR is that we will have to do some validation.

An interesting effect of the change in #57573 (WRITE executor changed to SAME executor on write action) that Henning pointed out is that bulk shard requests now have a task registered before execution on the WRITE whereas they previously did not have so. This changes the output of _tasks. This is not necessarily bad, but we should probably make sure we can distinguish between tasks that have executed and are waiting on replica and those that are just queued up (if we want to expose those). Can be handled on separate PR, just did not want to forget about that (i.e. think that needs to be addressed before #57573 goes into 7.9).

ywelsch · 2020-07-03T10:00:41Z

server/src/main/java/org/elasticsearch/action/index/IndexRequest.java

@@ -697,6 +697,6 @@ public long getAutoGeneratedTimestamp() {

    @Override
    public long ramBytesUsed() {
-        return SHALLOW_SIZE + RamUsageEstimator.sizeOf(id) + (source == null ? 0 : source.ramBytesUsed());
+        return SHALLOW_SIZE + RamUsageEstimator.sizeOf(id) + (source == null ? 0 : source.length());


While I think that this should be ok for now, it's critical to confirm though that our accounting is not completely off with some tests/benchmarks.

A first test to ensure that we are not vastly underaccounting should have a high indices.write.limit (e.g. 80%) and check that we don't hit the real-memory circuit breaker when we are filling up the node (should probably block requests from being processed), but rather the indices.write.limit instead.
A second test to ensure that we are not vastly overaccounting should have a low indices.write.limit, for example (e.g. 100MB), and show that we can still put a decently large number of requests to the system (i.e. send bulks that are in total making up 80MB in input size) without reaching the limit.
Third we need to check what the overhead of primaries is to wait on replica responses (i.e. memory consumed by listeners etc.). For that we should block processing on a replica, and fill up a primary and see how much memory is being consumed.

server/src/main/java/org/elasticsearch/action/bulk/WriteMemoryLimits.java

ywelsch · 2020-07-03T10:06:47Z

server/src/main/java/org/elasticsearch/action/bulk/WriteMemoryLimits.java


 import java.util.concurrent.atomic.AtomicLong;

 public class WriteMemoryLimits {

+    // TODO: Adjust
+    public static final Setting<ByteSizeValue> MAX_INDEXING_BYTES =
+        Setting.memorySizeSetting("indices.write.limit", "10%", Setting.Property.NodeScope, Setting.Property.Dynamic);


10% can be quite a lot for a system with a big heap (i.e. 3GB for a 32GB heap). Should we put an upper bound on this number? Can we run some benchmarks to determine what a good limit would look like (e.g. by checking the lowest limit we can get away with using our current Rally benchmarks).

I'm not sure what rally benchmarks you are referring to? All of the rally benchmarks by default are going to use very little indexing memory since they don't use many clients.

I ran some benchmarks today with 10K clients per node and have some numbers. But those a pretty specific to the security use case. We can talk about them tomorrow. But generally the CPUs were saturated with indexing and write queue latency was pretty high (ranging from 50ms - 2s through the benchmark.) In these benchmarks the write limits tended to be 200-300MB and the replica limits tended to be 10-80MB.

I'm not exactly sure what specific benchmarks you want here. I have run this with some concurrent security benchmarks and have a good idea about the write limits under various load. But our normally nightly rally benchmarks use very few clients and will not consume significant indexing memory.

…rite_limits

Tim-Brooks · 2020-07-06T23:40:02Z

I made some changes to this. Added a replica limit. And added a test to ensure we do not trip the write limits in unexpected places. But we should sync up so that we can agree on what specific validation needs to be performed on this PR.

…rite_limits

ywelsch

I've left one comment on the setting name and dynamicity

ywelsch · 2020-07-08T13:42:20Z

server/src/main/java/org/elasticsearch/action/bulk/WriteMemoryLimits.java


 import java.util.concurrent.atomic.AtomicLong;

 public class WriteMemoryLimits {

+    // TODO: Adjust
+    public static final Setting<ByteSizeValue> MAX_INDEXING_BYTES =
+        Setting.memorySizeSetting("indices.write.limit", "10%", Setting.Property.NodeScope, Setting.Property.Dynamic);


As discussed, let's make this a non-dynamic node setting. We also need to discuss whether we should white-list this setting in Cloud (so that users can change it there).

I think that we should not pick a setting name that is under the "indices" namespace, but perhaps introduce something completely new, for example indexing_limits.memory.limit

I changed the name and made it non-dynamic.

ywelsch · 2020-07-08T13:49:08Z

server/src/internalClusterTest/java/org/elasticsearch/action/bulk/WriteMemoryLimitsIT.java

            primaryTransportService.clearAllRules();
        }
    }
+
+    public void testWriteCanBeRejectedAtCoordinatingLevel() throws Exception {


As we discussed, we eventually want to have a test that also covers the rest layer.

I added a comment.

ywelsch

LGTM

This commit adds rejections when the indexing memory limits are exceeded for primary or coordinating operations. The amount of bytes allow for indexing is controlled by a new setting indexing_limits.memory.limit.

Tim-Brooks · 2020-07-17T19:09:34Z

The meta issue #59263 describes this work from a release highlight perspective.

Data frame analytics jobs that work with very large datasets may produce bulk requests that are over the memory limit for indexing. This commit adds a helper class that bundles index requests in bulk requests that steer away from the memory limit. We then use this class both from the results joiner and the inference runner ensuring data frame analytics jobs do not generate bulk requests that are too large. Note the limit was implemented in elastic#58885.

Data frame analytics jobs that work with very large datasets may produce bulk requests that are over the memory limit for indexing. This commit adds a helper class that bundles index requests in bulk requests that steer away from the memory limit. We then use this class both from the results joiner and the inference runner ensuring data frame analytics jobs do not generate bulk requests that are too large. Note the limit was implemented in #58885.

Data frame analytics jobs that work with very large datasets may produce bulk requests that are over the memory limit for indexing. This commit adds a helper class that bundles index requests in bulk requests that steer away from the memory limit. We then use this class both from the results joiner and the inference runner ensuring data frame analytics jobs do not generate bulk requests that are too large. Note the limit was implemented in elastic#58885. Backport of elastic#60219

…0283) Data frame analytics jobs that work with very large datasets may produce bulk requests that are over the memory limit for indexing. This commit adds a helper class that bundles index requests in bulk requests that steer away from the memory limit. We then use this class both from the results joiner and the inference runner ensuring data frame analytics jobs do not generate bulk requests that are too large. Note the limit was implemented in #58885. Backport of #60219

…0284) Data frame analytics jobs that work with very large datasets may produce bulk requests that are over the memory limit for indexing. This commit adds a helper class that bundles index requests in bulk requests that steer away from the memory limit. We then use this class both from the results joiner and the inference runner ensuring data frame analytics jobs do not generate bulk requests that are too large. Note the limit was implemented in #58885. Backport of #60219

Tim-Brooks added 2 commits July 1, 2020 14:23

WIP

397f78d

Tests

00d7057

Tim-Brooks added >enhancement :Distributed Indexing/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search. v8.0.0 v7.9.0 labels Jul 2, 2020

Tim-Brooks requested review from ywelsch and henningandersen July 2, 2020 00:51

elasticmachine added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label Jul 2, 2020

Fix tests

57fab74

Changes

1292822

This was referenced Jul 3, 2020

Add Bulk stats track the bulk per shard #52208

Merged

Indexing circuit breaker #52469

Closed

ywelsch reviewed Jul 3, 2020

View reviewed changes

Tim-Brooks added 3 commits July 6, 2020 09:57

Merge remote-tracking branch 'upstream/master' into reject_based_on_w…

c755989

…rite_limits

Changes

dbab82f

Change

cba3bb1

Tim-Brooks requested a review from ywelsch July 6, 2020 23:41

getsaurabh02 mentioned this pull request Jul 7, 2020

Creating effective back-pressure in ES Write Path #59116

Open

ywelsch added the release highlight label Jul 8, 2020

Merge remote-tracking branch 'upstream/master' into reject_based_on_w…

40b1eb7

…rite_limits

ywelsch reviewed Jul 8, 2020

View reviewed changes

Setting

cde58b9

Tim-Brooks requested a review from ywelsch July 8, 2020 14:11

ywelsch approved these changes Jul 8, 2020

View reviewed changes

Fix tests

243de35

Tim-Brooks merged commit 611fb03 into elastic:master Jul 8, 2020

Tim-Brooks added the backport pending label Jul 8, 2020

Tim-Brooks mentioned this pull request Jul 8, 2020

Improve pending indexing metrics and back pressure #59263

Open

13 tasks

Tim-Brooks removed the backport pending label Jul 13, 2020

ywelsch added release highlight and removed release highlight labels Jul 14, 2020

Tim-Brooks mentioned this pull request Jul 15, 2020

Reestablish peer recovery after network errors #55274

Merged

dimitris-athanasiou mentioned this pull request Jul 27, 2020

[ML] Ensure bulk requests are not over memory limit #60219

Merged

dimitris-athanasiou mentioned this pull request Jul 28, 2020

[7.x][ML] Ensure bulk requests are not over memory limit (#60219) #60283

Merged

dimitris-athanasiou mentioned this pull request Jul 28, 2020

[7.9][ML] Ensure bulk requests are not over memory limit (#60219) #60284

Merged

hendrikmuhs mentioned this pull request Jul 29, 2020

[Transform] Ensure bulk requests are not over memory limit - handle errors due to limits gracefully #60391

Open

danielmitterdorfer mentioned this pull request Dec 15, 2020

Account for Index Writer Cache in Circuit breaker #45648

Closed

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

shawnclarke7 mentioned this pull request Oct 20, 2022

elasticsearch_thread_pool_rejected_count no longer returns values (other than 0) prometheus-community/elasticsearch_exporter#638

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement rejections in `WriteMemoryLimits` #58885

Implement rejections in `WriteMemoryLimits` #58885

Tim-Brooks commented Jul 2, 2020 •

edited by ywelsch

Loading

elasticmachine commented Jul 2, 2020

Tim-Brooks commented Jul 2, 2020

ywelsch left a comment

ywelsch Jul 3, 2020

ywelsch Jul 3, 2020

Tim-Brooks Jul 6, 2020

Tim-Brooks Jul 6, 2020

Tim-Brooks commented Jul 6, 2020

ywelsch left a comment

ywelsch Jul 8, 2020

Tim-Brooks Jul 8, 2020

ywelsch Jul 8, 2020

Tim-Brooks Jul 8, 2020

ywelsch left a comment

Tim-Brooks commented Jul 17, 2020

Implement rejections in WriteMemoryLimits #58885

Implement rejections in WriteMemoryLimits #58885

Conversation

Tim-Brooks commented Jul 2, 2020 • edited by ywelsch Loading

elasticmachine commented Jul 2, 2020

Tim-Brooks commented Jul 2, 2020

ywelsch 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

Choose a reason for hiding this comment

Tim-Brooks commented Jul 6, 2020

ywelsch 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

Choose a reason for hiding this comment

ywelsch left a comment

Choose a reason for hiding this comment

Tim-Brooks commented Jul 17, 2020

Implement rejections in `WriteMemoryLimits` #58885

Implement rejections in `WriteMemoryLimits` #58885

Tim-Brooks commented Jul 2, 2020 •

edited by ywelsch

Loading