-
Notifications
You must be signed in to change notification settings - Fork 25.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Track queued merges in ElasticsearchMergeScheduler and InternalEngine #121794
Conversation
This commit adds tracking for merges that are queued for future execution. Relates ES-10570
Pinging @elastic/es-distributed-indexing (Team:Distributed Indexing) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -41,6 +41,9 @@ public class MergeTracking { | |||
private final Set<OnGoingMerge> onGoingMerges = ConcurrentCollections.newConcurrentSet(); | |||
private final Set<OnGoingMerge> readOnlyOnGoingMerges = Collections.unmodifiableSet(onGoingMerges); | |||
|
|||
private final Set<OnGoingMerge> queuedMerges = ConcurrentCollections.newConcurrentSet(); | |||
private final Set<OnGoingMerge> readOnlyQueuedMerges = Collections.unmodifiableSet(queuedMerges); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's an interesting idea! I guess we can piggy back on the fact that UnmodifiableCollection
is just a shell and the changes to queuedMerges
are guaranteed to be visible via the readOnlyQueuedMerges
reference.
return readOnlyQueuedMerges; | ||
} | ||
|
||
public void markMergeQueued(OnGoingMerge merge) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do see MergeTracking being used in stateless but also in stateful (InternalEngine). So I wonder why we do not call this function also for the stateful merges? An oversight? Or is it because merges cannot be queued in stateful and they just immediately execute?
If we do not mark queued merges in core ES, at least we should document it in the javadoc of markMergeQueued
, hasQueuedOrOnGoingMerges()
and hasQueuedMerges()
, so at least people are not misled to use the functions in stateful without understanding that queued merges are not there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or is it because merges cannot be queued in stateful and they just immediately execute?
In stateful merges get executed immediately in their own new thread (
Lines 136 to 143 in a8de554
@Override | |
protected MergeThread getMergeThread(MergeSource mergeSource, MergePolicy.OneMerge merge) throws IOException { | |
MergeThread thread = super.getMergeThread(mergeSource, merge); | |
thread.setName( | |
EsExecutors.threadName(indexSettings, "[" + shardId.getIndexName() + "][" + shardId.id() + "]: " + thread.getName()) | |
); | |
return thread; | |
} |
If we do not mark queued merges in core ES, at least we should document it in the javadoc of markMergeQueued, hasQueuedOrOnGoingMerges() and hasQueuedMerges(), so at least people are not misled to use the functions in stateful without understanding that queued merges are not there.
Sure, I'll add a javadoc and mention that it's implementation dependant 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh OK thanks for explaining! Yea javadoc to maybe explain what you just said would be nice so that people know in stateful there are no queued merges.
return readOnlyQueuedMerges; | ||
} | ||
|
||
public void markMergeQueued(OnGoingMerge merge) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh OK thanks for explaining! Yea javadoc to maybe explain what you just said would be nice so that people know in stateful there are no queued merges.
I'm closing this in favour of a simpler solution. |
This commit adds tracking for merges that are queued for future execution.
Relates ES-10570