Ref count write operations on IndexShard #10610
Conversation
| @@ -130,10 +130,9 @@ protected ShardIterator shards(ClusterState clusterState, InternalRequest reques | |||
| } | |||
|
|
|||
| @Override | |||
| protected Tuple<BulkShardResponse, BulkShardRequest> shardOperationOnPrimary(ClusterState clusterState, PrimaryOperationRequest shardRequest) { | |||
| protected Tuple<BulkShardResponse, BulkShardRequest> shardOperationOnPrimary(ClusterState clusterState, PrimaryOperationRequest shardRequest, IndexShard indexShard) { | |||
There was a problem hiding this comment.
I wonder if we should also pass the IndexService since we already have it in the base class. Saves on the lookup.
brwe
force-pushed
the
feature/index-shard-refcounting
branch
from
e169733
to
ed4e2bc
Compare
|
Addressed all comments. Want to take another look? |
| @@ -721,6 +725,9 @@ public void recover(Engine.RecoveryHandler recoveryHandler) throws EngineExcepti | |||
| } | |||
|
|
|||
| public void failShard(String reason, Throwable e) { | |||
| if (engineClosed()) { | |||
There was a problem hiding this comment.
why do we need to check this here I think we should never check this and just call fail on it?
There was a problem hiding this comment.
or if use engineUnsafe() and check for null
There was a problem hiding this comment.
I needed it for an Integration test I wrote, not sure if it makes sense though. See my comment above #10610 (comment)
|
I did another review of this |
| * Wipes any data that a test can leave behind: indices, templates and repositories | ||
| */ | ||
| public void wipe() { | ||
| assertShardIndexCounter(); |
|
I did another review...
I think we don't need any integration test here really, we have enough integration tests that test this and your assertions are good too? |
brwe
force-pushed
the
feature/index-shard-refcounting
branch
from
7b104ca
to
3f2bcd2
Compare
|
I rebased on master now that #10749 has been pushed and replaced all integration tests by unit tests similar to the ones in #10749. I like the new test much more than the old ones but I had some trouble simulating an IndexShard and am unsure if the way I do it is a little too hacky. Please have another look! |
|
Chatted with @s1monw and implemented all changes now. Want to have another look? |
| IndexService indexService = indicesService.indexServiceSafe(shardId.index().getName()); | ||
| IndexShard indexShard = indexService.shardSafe(shardId.id()); | ||
| return new IndexShardReference(indexShard); | ||
|
|
brwe
force-pushed
the
feature/index-shard-refcounting
branch
from
1eed609
to
352476b
Compare
brwe
force-pushed
the
feature/index-shard-refcounting
branch
from
352476b
to
e47e1ef
Compare
|
pushed another commit |
|
LGTM |
brwe
added
v2.0.0-beta1
:Core/Infra/Core
|
still have to backport to 1.x... |
This commit adds a counter for IndexShard that keeps track of how many write operations are currently in flight on a shard. The counter is incremented whenever a write request is submitted in TransportShardReplicationOperationAction and decremented when it is finished. On a primary it stays incremented while replicas are being processed. The counter is an instance of AbstractRefCounted. Once this counter reaches 0 each write operation will be rejected with an IndexClosedException. closes elastic#10610
This commit adds a counter for IndexShard that keeps track of how many write operations are currently in flight on a shard. The counter is incremented whenever a write request is submitted in TransportShardReplicationOperationAction and decremented when it is finished. On a primary it stays incremented while replicas are being processed. The counter is an instance of AbstractRefCounted. Once this counter reaches 0 each write operation will be rejected with an IndexShardClosedException. closes #10610
|
pushed to 1.x |
clintongormley
changed the title
This commit adds a counter for IndexShard that keeps track of how many write operations
are currently in flight on a shard. The counter is incremented whenever a write request is submitted in TransportShardReplicationOperationAction and decremented when it is finished. On a primary it stays incremented while replicas are being processed. The counter is an instance of AbstractRefCounted. Once this counter reaches 0 each write operation will be rejected with an IndexShardClosedException.
As a follow up we could use this counter to block closing of a shard until all pending write operations have been processed. Currently we only decrement the counter once on IndexShard.close() but do not wait for the counter to reach 0.