Account for time taken to write index buffers in IndexingMemoryController#126786
Account for time taken to write index buffers in IndexingMemoryController#126786ankikuma merged 30 commits intoelastic:mainfrom
Conversation
elasticsearchmachine
added
needs:triage
ankikuma
added
the
Team:Distributed Indexing (obsolete)
elasticsearchmachine
removed
the
Team:Distributed Indexing (obsolete)
ankikuma
requested review from
PeteGillinElastic,
fcofdez and
henningandersen
and removed request for
PeteGillinElastic and
fcofdez
ankikuma
added
the
:Distributed/Distributed
elasticsearchmachine
added
Team:Distributed Indexing (obsolete)
|
Hi @ankikuma, I've created a changelog YAML for you. |
|
Pinging @elastic/es-distributed-indexing (Team:Distributed Indexing) |
…ch into 04142025/IWLES11356 commit pull
fcofdez
left a comment
fcofdez
left a comment
There was a problem hiding this comment.
Looks good, can we add a test where we exercise this and see how the write load increases when the IndexingMemoryController flushes the buffers to disk?
server/src/main/java/org/elasticsearch/indices/IndexingMemoryController.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/shard/InternalIndexingStats.java
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/shard/InternalIndexingStats.java
Outdated
Show resolved
Hide resolved
|
@fcofdez I added a test to IndexingMemoryControllerIT to verify that the stats for total execution time (which includes the time taken to write indexing buffers) are collected and are indeed greater than just the indexing time alone. Please take a look when you get a chance. |
fcofdez
left a comment
fcofdez
left a comment
There was a problem hiding this comment.
Thanks for addressing the feedback, there're a couple of small things to tackle before we merge this 👍 .
| @@ -1,14 +0,0 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
There was a problem hiding this comment.
I think that this deletion was accidental? can we revert these changes?
There was a problem hiding this comment.
I had deleted the .idea directory from my elasticsearch repo to fix IntelliJ suddenly not recognizing any Lucene libraries, this is an effect of that. will fix it.
server/src/main/java/org/elasticsearch/index/shard/InternalIndexingStats.java
Outdated
Show resolved
Hide resolved
| IndexService indexService = createIndex("index", indexSettings(1, 0).put("index.refresh_interval", -1).build()); | ||
| IndexShard shard = indexService.getShard(0); | ||
| for (int i = 0; i < 100; i++) { | ||
| prepareIndex("index").setId(Integer.toString(i)).setSource("field", "value").get(); |
There was a problem hiding this comment.
nit: we don't need to set the id explicitly or at least we should add ids randomly, since org.elasticsearch.index.engine.InternalEngine#writeIndexingBuffer takes into account the version map size too (which is populated when explicit ids are used).
| public void testIndexingUpdatesRelevantStats() throws Exception { | ||
| IndexService indexService = createIndex("index", indexSettings(1, 0).put("index.refresh_interval", -1).build()); | ||
| IndexShard shard = indexService.getShard(0); | ||
| for (int i = 0; i < 100; i++) { |
There was a problem hiding this comment.
nit: maybe we can do this in a single bulk request?
There was a problem hiding this comment.
Actually we don't even need to index so many documents. The stat goes up in every request.
server/src/internalClusterTest/java/org/elasticsearch/indices/IndexingMemoryControllerIT.java
Outdated
Show resolved
Hide resolved
|
@fcofdez Should I mark this for backport ? |
fcofdez
left a comment
fcofdez
left a comment
There was a problem hiding this comment.
LGTM. There's a pending comment about a method naming suggestion, but you can merge the PR once that's addressed. Thanks for all the iterations on this 👍
| * @param took time it took to write the index buffers for this shard | ||
| * @see org.elasticsearch.indices.IndexingMemoryController | ||
| */ | ||
| public void writeIndexBuffersOnIndexThreads(long took) { |
There was a problem hiding this comment.
can we rename this method to addWriteIndexBuffersOnIndexThreadsTime? its current naming is a bit confusing as it implies that it would do the write. Also, can we include the unit in the parameter method? i.e. tookInNanos?
3 participants
Seeks to address ES-11356.
This PR adds to the indexing write load, the time taken to flush write indexing buffers using the indexing threads (this is done here to push back on indexing)
This changes the semantics of
InternalIndexingStats#recentIndexMetricandInternalIndexingStats#peakIndexMetricto more accurately account for load on the indexing thread.