Remove TranslogService and fold it into synchronous IndexShard API #13707
Conversation
| indexShard.sync(location); | ||
| } | ||
| if (indexShard.flushPending()) { | ||
| threadPool.executor(ThreadPool.Names.FLUSH).execute(() -> { |
There was a problem hiding this comment.
this can create a storm of tasks pending to be executed on the thread pool. For example, if a flush is needed, and we are doing 5k indexing requests per second, until the thread get scheduled, and the flush actually executes to "cleanup" the flushPending (can take time), there will be thousands of tasks pending on the thread pool, eventually executing on a thread and potentially not doing anything. If active indexing is happening all the time and flushing is continuously needed, we will have an exponential number of thread switches to execute a flush.
Might it make sense to have an AtomicBoolean like behavior encapsulated in IndexShard? an "executing flush" like semantics?
|
I'm +1 on removing the time component for translog flushing which leaves translog service without any real value. Folding what's left into index shard also makes sense. I'm a bit concerned that flushing is now left to the TransportReplicationAction. It doesn't feel like that's where this logic should be as the replication action doesn't really need to know what the index shard is doing with the write operations. Instead I proposed adding this logic to index shard it self, where all write operations go through as well. I share Shay's concern about the flush storm that may happen. In the spirit of the new java 8 lambda functionality it feels like we need a debounce utility :) |
I should have added this to the issue |
|
I pushed a new commit that prevent the storms |
| asyncFlushRunning.compareAndSet(true, false); | ||
| } | ||
| }; | ||
| if (shouldFlush()) { |
There was a problem hiding this comment.
this double check seems redundant? we check it right before in the first line of the method? will simplify a bit the code, since there is no need for an else?
|
left a minor comment, LGTM otherwise |
|
LGTM2. Agreed with Shay's comment. |
s1monw
force-pushed
the
shard_level_services
branch
from
8dcdf15
to
20579c6
Compare
This commit moves the size and ops based flush into a synchronous API into IndexShard and removes the time-based flush alltogether since it' basically covered by the inactive async flush API we have today. The functionality doesn't need to be covered by scheduled task and async APIs while we can actually make all the decisions in a sync manner which is way easier to control and to test. Closes elastic#13707
s1monw
force-pushed
the
shard_level_services
branch
from
20579c6
to
75e8164
Compare
The check prevents a race condition since we can't use real locks here. Relates to #13707
… marked as inactive The IndexingMemoryController checks periodically if there is any indexing activity on the shard. If no activity is sean for 5m (default) the shard is marked as inactive allowing it's indexing buffer quota to given to other active shards. Sadly the current check is bad as it checks for 0 translog operation. This makes the inactive wait for a flush to happen - which used to take 30m and since elastic#13707 doesn't happen at all (as we rely on the synced flush triggered by inactivity). This commit fixes the check so it will work with any translog size.
… marked as inactive The IndexingMemoryController checks periodically if there is any indexing activity on the shard. If no activity is sean for 5m (default) the shard is marked as inactive allowing it's indexing buffer quota to given to other active shards. Sadly the current check is bad as it checks for 0 translog operation. This makes the inactive wait for a flush to happen - which used to take 30m and since #13707 doesn't happen at all (as we rely on the synced flush triggered by inactivity). This commit fixes the check so it will work with any translog size. Closes #13759
clintongormley
added
:Distributed/Distributed
This commit moves the size and ops based flush into a synchronous API into
IndexShard and removes the time-based flush alltogether since it' basically
covered by the inactive async flush API we have today. The functionality doesn't
need to be covered by scheduled task and async APIs while we can actually make all
the decisions in a sync manner which is way easier to control and to test.