[CCR] Added write buffer size limit #34797

martijnvg · 2018-10-24T10:20:46Z

This limit is based on the size in bytes of the operations in the write buffer. If this limit is exceeded then no more read operations will be coordinated until the size in bytes of the write buffer has dropped below the configured write buffer size limit.

Renamed existing max_write_buffer_size to max_write_buffer_count to indicate that limit is count based.

Closes #34705

This limit is based on the size in bytes of the operations in the write buffer. If this limit is exceeded then no more read operations will be coordinated until the size in bytes of the write buffer has dropped below the configured write buffer size limit. Renamed existing `max_write_buffer_size` to ``max_write_buffer_count` to indicate that limit is count based. Closes elastic#34705

elasticmachine · 2018-10-24T10:20:48Z

Pinging @elastic/es-distributed

dnhatn

This looks great. I left some minor comments.

I noticed that the current implementation (not this PR) might fill the write buffer more than the limit. When we send a read-request, we don't limit the request count and size to the vacant slots of the write buffer. If the buffer has one byte (or one count) left, we still issue a full read-request. We can fix this in a follow-up if we feel we should.

dnhatn · 2018-10-24T13:36:07Z

x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/AutoFollowCoordinator.java

@@ -327,7 +327,7 @@ private void followLeaderIndex(String autoFollowPattenName,
            followRequest.setMaxConcurrentReadBatches(pattern.getMaxConcurrentReadBatches());
            followRequest.setMaxBatchSize(pattern.getMaxBatchSize());
            followRequest.setMaxConcurrentWriteBatches(pattern.getMaxConcurrentWriteBatches());
-            followRequest.setMaxWriteBufferSize(pattern.getMaxWriteBufferSize());
+            followRequest.setMaxWriteBufferCount(pattern.getMaxWriteBufferCount());


We forget passing "maxWriteBufferSize" parameter.

fixed: 7af8ba2

dnhatn · 2018-10-24T13:36:42Z

x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/ShardFollowNodeTask.java

@@ -85,6 +85,7 @@
    private long numberOfOperationsIndexed = 0;
    private long lastFetchTime = -1;
    private final Queue<Translog.Operation> buffer = new PriorityQueue<>(Comparator.comparing(Translog.Operation::seqNo));
+    private long bufferSize = 0;


Should it be bufferSizeInBytes?

renamed: 9925984

dnhatn · 2018-10-24T13:38:26Z

x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/ShardFollowNodeTask.java

@@ -208,6 +213,8 @@ private synchronized void coordinateWrites() {
                    break;
                }
            }
+            long opsSize = ops.stream().mapToLong(Translog.Operation::estimateSize).sum();


Can we move this into the loop to avoid the loop.

removed: 980f1a5

martijnvg · 2018-10-24T14:29:30Z

@dnhatn Thanks for reviewing!

I noticed that the current implementation (not this PR) might fill the write buffer more than the limit. When we send a read-request, we don't limit the request count and size to the vacant slots of the write buffer. If the buffer has one byte (or one count) left, we still issue a full read-request.

Yes, that is what it is currently doing. I don't see a real problem in the way the limit is currently enforced.

bleskes · 2018-10-24T14:35:51Z

Yes, that is what it is currently doing. I don't see a real problem in the way the limit is currently enforced.

+1. The main rationale here was to keep things simple and treat the limit as a soft limit that is used to cause back pressure. No more.

dnhatn

LGTM

dnhatn · 2018-10-24T15:12:52Z

x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/ShardFollowNodeTask.java

@@ -455,6 +466,7 @@ public synchronized ShardFollowNodeTaskStatus getStatus() {
                numConcurrentReads,
                numConcurrentWrites,
                buffer.size(),
+            bufferSizeInBytes,


nit: indentation

dnhatn · 2018-10-24T15:13:40Z

Makes sense. Thanks @martijnvg and @bleskes for explaining.

martijnvg · 2018-10-24T18:38:07Z

Note that this PR also changes the default for max_write_buffer_count from 10240 to unlimited:
9d642b5

The reason behind this that a finding a default for this parameter is difficult and because this pr add max_write_buffer_size, that will make sure that the write buffer does not accumulate too many write operations.

jasontedor

LGTM.

This limit is based on the size in bytes of the operations in the write buffer. If this limit is exceeded then no more read operations will be coordinated until the size in bytes of the write buffer has dropped below the configured write buffer size limit. Renamed existing `max_write_buffer_size` to ``max_write_buffer_count` to indicate that limit is count based. Closes #34705

martijnvg added >non-issue :Distributed Indexing/CCR Issues around the Cross Cluster State Replication features labels Oct 24, 2018

martijnvg requested review from bleskes and jasontedor October 24, 2018 10:20

martijnvg added 2 commits October 24, 2018 13:59

Merge remote-tracking branch 'es/master' into ccr_max_write_buffer_size

a4dd5b0

fixed tests

90218c0

dnhatn reviewed Oct 24, 2018

View reviewed changes

martijnvg added 4 commits October 24, 2018 16:16

Merge remote-tracking branch 'es/master' into ccr_max_write_buffer_size

1b25cf2

pass down maxWriteBufferSize

7af8ba2

rename

9925984

reuse sumEstimatedSize

980f1a5

dnhatn approved these changes Oct 24, 2018

View reviewed changes

martijnvg added 3 commits October 24, 2018 18:47

increase default to disable by default the buffer count limit

9d642b5

fix indent

96e20b1

Merge remote-tracking branch 'es/master' into ccr_max_write_buffer_size

122343b

dliappis mentioned this pull request Oct 24, 2018

Rename max_write_buffer_size in _ccr/stats as it's misleading #33906

Closed

Merge remote-tracking branch 'es/master' into ccr_max_write_buffer_size

515968d

jasontedor approved these changes Oct 24, 2018

View reviewed changes

fixed test

163e709

dliappis mentioned this pull request Oct 24, 2018

[CCR] Re-evaluate shard follow parameter defaults #31717

Closed

martijnvg merged commit 6fe0e62 into elastic:master Oct 24, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CCR] Added write buffer size limit #34797

[CCR] Added write buffer size limit #34797

martijnvg commented Oct 24, 2018 •

edited

Loading

elasticmachine commented Oct 24, 2018

dnhatn left a comment

dnhatn Oct 24, 2018

martijnvg Oct 24, 2018

martijnvg Oct 24, 2018

dnhatn Oct 24, 2018

martijnvg Oct 24, 2018

dnhatn Oct 24, 2018

martijnvg Oct 24, 2018

martijnvg commented Oct 24, 2018

bleskes commented Oct 24, 2018

dnhatn left a comment

dnhatn Oct 24, 2018

dnhatn commented Oct 24, 2018

martijnvg commented Oct 24, 2018

jasontedor left a comment

[CCR] Added write buffer size limit #34797

[CCR] Added write buffer size limit #34797

Conversation

martijnvg commented Oct 24, 2018 • edited Loading

elasticmachine commented Oct 24, 2018

dnhatn 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martijnvg commented Oct 24, 2018

bleskes commented Oct 24, 2018

dnhatn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnhatn commented Oct 24, 2018

martijnvg commented Oct 24, 2018

jasontedor left a comment

Choose a reason for hiding this comment

martijnvg commented Oct 24, 2018 •

edited

Loading