Online prewarming service interface docs and usage in SearchService #126561
Conversation
This adds the interface for search online prewarming with a default NOOP implementation. This also hooks the interface in the SearchService after we fork the query phase to the search thread pool.
andreidan
added
>non-issue
:Search Foundations/Search
elasticsearchmachine
added
the
Team:Search Foundations
|
Pinging @elastic/es-search-foundations (Team:Search Foundations) |
| * should be skipped and indicate this decision via this | ||
| * flag. | ||
| */ | ||
| void prewarm(IndexShard indexShard, boolean skipPrewarming); |
There was a problem hiding this comment.
Curious to understand why we are passing a skip flag here instead of letting the caller to an if-block around the prewarm() call?
There was a problem hiding this comment.
I thought it'd encourage callers to think about skipping under some circumstance that pertains to the caller (_search is concerned with what happens in the search threadpool, _query might derive other heuristics).
I'm happy to remove it and wrap the calls in if/else if it's currently confusing ?
WDYT ?
There was a problem hiding this comment.
Yeah, I would expect a flag like this only if it modifies the behavior of execution (i.e. orderWhisky(boolean withIce)) but not when it is intended to skip the purpose of the method call itself (i.e. orderWhisky(boolean damnINeedToDrive). I get the "encouragement to think" part but I would prefer to see that really nothing happens under certain circumstances on the caller site already. We have this being a no-op in stateful on top of this still.
| // we successfully submitted the async task to the search pool so let's prewarm the shard | ||
| onlinePrewarmingService.prewarm( | ||
| shard, | ||
| executor instanceof ThreadPoolExecutor tpe |
There was a problem hiding this comment.
You probably did some digging around why to do this type check here. Since its a skip condition, maybe worth explaining somewhere.
There was a problem hiding this comment.
Nevermind, I see we skip conditioning on the queue size which seems to be a specific thing for tpe. I was probably put of by this being an instanceof check instead of a simple cast. I mostly wondered if we cannot be sure that executor here is always a TPE or maybe we have other cases in tests etc...
| onlinePrewarmingService.prewarm( | ||
| readerContext.indexShard(), | ||
| executor instanceof ThreadPoolExecutor tpe | ||
| && ((tpe.getMaximumPoolSize() * prewarmingMaxPoolFactorThreshold) < tpe.getQueue().size()) |
There was a problem hiding this comment.
since the skip condition seems to be the same everywhere, maybe a good candidate for a short helper function? That would also maybe be a good spot for additional short docs on the condition.
| // the system has a chance to catch up and prewarming doesn't take over the network bandwidth | ||
| public static final Setting<Integer> PREWARMING_THRESHOLD_THREADPOOL_SIZE_FACTOR_POOL_SIZE = Setting.intSetting( | ||
| "search.online_prewarming_threshold_poolsize_factor", | ||
| 10, |
There was a problem hiding this comment.
Any particular reason for this default that would be worth putting in the docs? Or is this something we are expecting to experiment with?
There was a problem hiding this comment.
++ we'll see what we end up with once we experiment and monitor a bit
server/src/main/java/org/elasticsearch/search/SearchService.java
Outdated
Show resolved
Hide resolved
This adds the interface for search online prewarming with a default NOOP implementation.
This also hooks the interface in the SearchService after we fork the query phase to the search thread pool.