When shard becomes active again, immediately increase its indexing buffer #13918

mikemccand · 2015-10-03T21:33:57Z

Spinoff from #13802.

Today, when an index goes from inactive to active, because indexing ops suddenly arrive to an inactive index, we (IndexingMemoryController) take up to 30 seconds to notice this and then increase the indexing buffer from the tiny idle 512KB to "its fair share". This is somewhat dangerous because it could write many, many segments in those 30 seconds...

This PR changes that so the inactive -> active transition instead causes us to immediately re-visit the indexing buffer for all shards.

It also shifts responsibility of tracking active/inactive into IndexShard, and no longer uses the translog ID/ops count to check for changes (I think this is more error prone? E.g. #13802), simplifying it instead to timestamp (System.nanoTime) of the last indexing op.

nik9000 · 2015-10-05T18:11:12Z

core/src/main/java/org/elasticsearch/index/shard/IndexShard.java

@@ -975,6 +999,7 @@ public void addFailedEngineListener(Engine.FailedEngineListener failedEngineList
        this.failedEngineListener.delegates.add(failedEngineListener);
    }

+    /** Returns true if the indexing buffer size did change */
    public void updateBufferSize(ByteSizeValue shardIndexingBufferSize, ByteSizeValue shardTranslogBufferSize) {


I don't think this doc is correct - the return type is void

Conflicts: core/src/main/java/org/elasticsearch/index/shard/IndexShard.java core/src/main/java/org/elasticsearch/index/shard/ShadowIndexShard.java core/src/main/java/org/elasticsearch/indices/memory/IndexingMemoryController.java

mikemccand · 2015-10-06T08:54:24Z

Thanks @nik9000 I folded in the feedback.

bleskes · 2015-10-06T09:13:05Z

core/src/main/java/org/elasticsearch/index/engine/ShadowEngine.java

+    @Override
+    public long indexWriterRAMBytesUsed() {
+        // No IndexWriter
+        return 0L;


I wonder if UnsupportedOperationException is a better choice here- we don't have an indexwriter...

bleskes · 2015-10-06T11:13:22Z

I love how this turned out, especially around moving away from the translog checks. I left some suggestions here and there. I also think we should move the controller to work on IndexShard objects rather than shard ids (sorry for the scope creep - can be done in a different change). At some point I got worried that a new shard will not be seen as "added" if the original shard was destroyed and a new one created. I think it's OK now (because we only use current state when allocating memory) but it's more correct to use physical instances.

mikemccand · 2015-10-06T20:51:02Z

I also think we should move the controller to work on IndexShard objects rather than shard ids

++

But I thought we used ShardId all over to make this more easily unit-tested? But anyway let's do this on a separate change ... this one is big already :)

mikemccand · 2015-10-06T21:20:00Z

OK I folded feedback @bleskes, thank you!

bleskes · 2015-10-07T11:47:18Z

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

@@ -288,6 +288,7 @@ public synchronized IndexShard createShard(int sShardId, ShardRouting routing) {
            indicesLifecycle.afterIndexShardCreated(indexShard);
            settingsService.addListener(indexShard);
            shards = newMapBuilder(shards).put(shardId.id(), indexShard).immutableMap();
+            indexServicesProvider.getIndexingMemoryController().forceCheck();


I was thinking about it and I think we should start the shard as inactive (it is :)) and only mark it as active + the claiming of memory in IndexShard#skipTranslogRecovery / IndexShard#performTranslogRecovery when we actually open the engine for indexing. We also make sure call IndexingMemoryController.forceCheck before we create the engine, so we always have an up to date buffer size ...

Re concerns about n^2 problem, it is there but since we limit the amount of concurrent recoveries a node can do, it means it will be done during a longer process. Also because of the limit it is hard to do anything around batching. All in all, I think we're OK.

Actually, why not simply start up as inactive, init the buffers to the INACTIVE values, and then the first indexing op that arrives will make it active like normal?

OK that's a good point, that concurrent recoveries are limited anyway, so the O(N^2) cost is paid over a long time.

Actually, why not simply start up as inactive, init the buffers to the INACTIVE values, and then the first indexing op that arrives will make it active like normal?

That was my original thought as well, but then I remembered you were very worried about indexing into a small buffer. When one creates an index by indexing into it that index will likely get many indexing requests at once, the first will trigger a forced check and the others will fly through into the engine. Figured we better not have to worry about it but If you're good with that, I'm good :)

If you're good with that, I'm good :)

I'm gonna try :)

bleskes · 2015-10-07T11:52:00Z

This is getting great. I left some final comments.

nik9000 · 2015-10-07T13:26:09Z

core/src/main/java/org/elasticsearch/index/shard/IndexShard.java

+            if (iwBytesUsed > shardIndexingBufferSize.bytes()) {
+                // our allowed buffer was changed to less than we are currently using; we ask IW to refresh
+                // so it clears its buffers (otherwise it won't clear until the next indexing/delete op)
+                logger.debug(message + "; now refresh to clear IndexWriter memory");


I'm used to wrapping debug logging stuff in if (logger.isDebugEnabled()) tests to prevent the message construction in the (very common) case that debug is disabled. Here it probably doesn't matter because message construction is dwarfed by the refresh call.

mikemccand · 2015-10-07T14:20:59Z

OK thanks @nik9000 and @bleskes, I folded in the last round of feedback.

bleskes · 2015-10-07T16:13:55Z

LGTM. Awesome mike.

Conflicts: core/src/main/java/org/elasticsearch/index/engine/Engine.java core/src/main/java/org/elasticsearch/index/shard/IndexShard.java

When shard becomes active again, immediately increase its indexing buffer instead of waiting for up to 30 seconds while indexing with a tiny (500 KB) indexing buffer.

When shard becomes active again, immediately increase its indexing buffer instead of waiting for up to 30 seconds while indexing with a tiny (500 KB) indexing buffer. Conflicts: core/src/main/java/org/elasticsearch/index/IndexServicesProvider.java core/src/main/java/org/elasticsearch/index/shard/IndexShard.java core/src/main/java/org/elasticsearch/index/shard/ShadowIndexShard.java core/src/main/java/org/elasticsearch/indices/memory/IndexingMemoryController.java core/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java core/src/test/java/org/elasticsearch/indices/memory/IndexingMemoryControllerIT.java

mikemccand added 3 commits October 3, 2015 05:09

a start

f27c0ad

fix tests; pull out translog buffer size constant

934cc09

set last indexing time before invoking IMC

19ab16b

mikemccand added >bug :Core/Infra/Core Core issues without another label v2.1.0 v5.0.0-alpha1 v2.2.0 labels Oct 3, 2015

mikemccand self-assigned this Oct 3, 2015

nik9000 reviewed Oct 5, 2015
View reviewed changes

mikemccand added 3 commits October 6, 2015 03:59

fold feedback

c597127

Merge branch 'master' into immediate_shard_active

a082135

Conflicts: core/src/main/java/org/elasticsearch/index/shard/IndexShard.java core/src/main/java/org/elasticsearch/index/shard/ShadowIndexShard.java core/src/main/java/org/elasticsearch/indices/memory/IndexingMemoryController.java

improve javadocs

0aabfe3

bleskes reviewed Oct 6, 2015
View reviewed changes

fold feedback

934827d

bleskes reviewed Oct 7, 2015
View reviewed changes

nik9000 reviewed Oct 7, 2015
View reviewed changes

feedback

7f435e2

Merge branch 'master' into immediate_shard_active

23f97c3

Conflicts: core/src/main/java/org/elasticsearch/index/engine/Engine.java core/src/main/java/org/elasticsearch/index/shard/IndexShard.java

mikemccand merged commit 9688e86 into elastic:master Oct 8, 2015

jasontedor mentioned this pull request Dec 4, 2015

IndexingMemoryController should not track shard index states #15251

Merged

mikemccand mentioned this pull request Jan 25, 2016

Translog replay is very slow because indexing buffer is stuck at 500 KB (inactive) #16206

Closed

mikemccand mentioned this pull request Feb 17, 2016

make EngineConfig.INACTIVE_SHARD_INDEXING_BUFFER configurable for ES-1.7.x #16688

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

When shard becomes active again, immediately increase its indexing buffer #13918

When shard becomes active again, immediately increase its indexing buffer #13918

mikemccand commented Oct 3, 2015

nik9000 Oct 5, 2015

mikemccand commented Oct 6, 2015

bleskes Oct 6, 2015

bleskes commented Oct 6, 2015

mikemccand commented Oct 6, 2015

mikemccand commented Oct 6, 2015

bleskes Oct 7, 2015

mikemccand Oct 7, 2015

bleskes Oct 7, 2015

mikemccand Oct 7, 2015

bleskes commented Oct 7, 2015

nik9000 Oct 7, 2015

mikemccand commented Oct 7, 2015

bleskes commented Oct 7, 2015

When shard becomes active again, immediately increase its indexing buffer #13918

When shard becomes active again, immediately increase its indexing buffer #13918

Conversation

mikemccand commented Oct 3, 2015

nik9000 Oct 5, 2015

Choose a reason for hiding this comment

mikemccand commented Oct 6, 2015

bleskes Oct 6, 2015

Choose a reason for hiding this comment

bleskes commented Oct 6, 2015

mikemccand commented Oct 6, 2015

mikemccand commented Oct 6, 2015

bleskes Oct 7, 2015

Choose a reason for hiding this comment

mikemccand Oct 7, 2015

Choose a reason for hiding this comment

bleskes Oct 7, 2015

Choose a reason for hiding this comment

mikemccand Oct 7, 2015

Choose a reason for hiding this comment

bleskes commented Oct 7, 2015

nik9000 Oct 7, 2015

Choose a reason for hiding this comment

mikemccand commented Oct 7, 2015

bleskes commented Oct 7, 2015