Skip to content

Account for time taken to write index buffers in IndexingMemoryController #126786

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 30 commits into from
May 1, 2025
Merged

Account for time taken to write index buffers in IndexingMemoryController #126786

merged 30 commits into from
May 1, 2025

Conversation

ankikuma
Copy link
Contributor

@ankikuma ankikuma commented Apr 14, 2025 •
edited
Loading

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#recentIndexMetric and InternalIndexingStats#peakIndexMetric to more accurately account for load on the indexing thread.

@elasticsearchmachine elasticsearchmachine added needs:triage Requires assignment of a team area label v9.1.0 labels Apr 14, 2025
@ankikuma ankikuma added the Team:Distributed Indexing Meta label for Distributed Indexing team label Apr 16, 2025
@elasticsearchmachine elasticsearchmachine removed the Team:Distributed Indexing Meta label for Distributed Indexing team label Apr 16, 2025
@ankikuma ankikuma requested review from fcofdez, PeteGillinElastic and henningandersen and removed request for fcofdez and PeteGillinElastic April 17, 2025 00:04
@ankikuma ankikuma added the :Distributed Indexing/Distributed A catch all label for anything in the Distributed Indexing Area. Please avoid if you can. label Apr 18, 2025
@elasticsearchmachine elasticsearchmachine added Team:Distributed Indexing Meta label for Distributed Indexing team and removed needs:triage Requires assignment of a team area label labels Apr 18, 2025
@elasticsearchmachine
Copy link
Collaborator

Hi @ankikuma, I've created a changelog YAML for you.

@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-distributed-indexing (Team:Distributed Indexing)

fcofdez
fcofdez reviewed Apr 22, 2025
View reviewed changes
Copy link
Contributor

@fcofdez fcofdez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@ankikuma ankikuma requested a review from a team as a code owner April 28, 2025 23:34
@ankikuma
Copy link
Contributor Author

@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
fcofdez reviewed Apr 30, 2025
View reviewed changes
Copy link
Contributor

@fcofdez fcofdez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing the feedback, there're a couple of small things to tackle before we merge this 👍 .

.idea/eclipseCodeFormatter.xml Outdated
@@ -1,14 +0,0 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this deletion was accidental? can we revert these changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/internalClusterTest/java/org/elasticsearch/indices/IndexingMemoryControllerIT.java Outdated
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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see. Thanks!

server/src/internalClusterTest/java/org/elasticsearch/indices/IndexingMemoryControllerIT.java Outdated
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++) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: maybe we can do this in a single bulk request?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually we don't even need to index so many documents. The stat goes up in every request.

@ankikuma
Copy link
Contributor Author

ankikuma commented May 1, 2025

@fcofdez Should I mark this for backport ?

fcofdez
fcofdez approved these changes May 1, 2025
View reviewed changes
Copy link
Contributor

@fcofdez fcofdez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍

server/src/main/java/org/elasticsearch/index/shard/IndexShard.java Outdated
* @param took time it took to write the index buffers for this shard
* @see org.elasticsearch.indices.IndexingMemoryController
*/
public void writeIndexBuffersOnIndexThreads(long took) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@ankikuma ankikuma merged commit 084542a into elastic:main May 1, 2025
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fcofdez fcofdez fcofdez approved these changes

@henningandersen henningandersen Awaiting requested review from henningandersen

@PeteGillinElastic PeteGillinElastic Awaiting requested review from PeteGillinElastic

Assignees
No one assigned
Labels
:Distributed Indexing/Distributed A catch all label for anything in the Distributed Indexing Area. Please avoid if you can. >enhancement Team:Distributed Indexing Meta label for Distributed Indexing team v9.1.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ankikuma @elasticsearchmachine @fcofdez