Skip to content

Testing indices query cache memory stats#135298

Merged
masseyke merged 17 commits intoelastic:mainfrom
masseyke:testing-IndicesQueryCache-memory-stats
Sep 26, 2025
Merged

Testing indices query cache memory stats#135298
masseyke merged 17 commits intoelastic:mainfrom
masseyke:testing-IndicesQueryCache-memory-stats

Conversation

@masseyke
Copy link
Copy Markdown
Member

@masseyke masseyke commented Sep 23, 2025

This adds a test to show how we expect the IndicesQueryCache to attribute memory to different shards. We attribute memory to a shard proportionally to how many documents in that shard are in the cache. This does not change behavior at all, but does change some variable names and comments for clarity.

@masseyke masseyke requested a review from joegallo September 23, 2025 16:31
@masseyke masseyke added >test Issues or PRs that are addressing/adding tests :Data Management/Indices APIs DO NOT USE. Use ":Distributed/Indices APIs" or ":StorageEngine/Templates" instead. v9.2.0 labels Sep 23, 2025
@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

elasticsearchmachine commented Sep 23, 2025

Pinging @elastic/es-data-management (Team:Data Management)

@elasticsearchmachine elasticsearchmachine added the Team:Data Management (obsolete) DO NOT USE. This team no longer exists. label Sep 23, 2025
@joegallo
Copy link
Copy Markdown
Contributor

joegallo commented Sep 23, 2025

We attribute memory to a shard proportionally to how many documents in that shard are in the cache.

I think it's all based on the number of segments, not the number of documents.

@masseyke
Copy link
Copy Markdown
Member Author

masseyke commented Sep 24, 2025 •
edited
Loading

We attribute memory to a shard proportionally to how many documents in that shard are in the cache.

I think it's all based on the number of segments, not the number of documents.

OK I looked more closely, and you're right. I rewrote the unit test and was able to see that (generally) memory is attributed proportionally to the number of cached queries per segment. However, the cache stats vary so much that I cannot get a meaningful test to reliably pass. I think I have uncovered several bugs in these cache stats (or at the very least, several big gaps in my understanding about what they are capturing). I think I'm going to close this PR because fixing a lot of bugs in the cache stats is way outside of the scope of what I'm intending to do right now.

@masseyke masseyke marked this pull request as draft September 25, 2025 23:09
@masseyke masseyke marked this pull request as ready for review September 26, 2025 16:15
masseyke added a commit to masseyke/elasticsearch that referenced this pull request Sep 26, 2025
@masseyke
making sure it works with changes from elastic#135298
9414544
@joegallo
Copy link
Copy Markdown
Contributor

joegallo commented Sep 26, 2025

This might give us a little trouble if/when we turn on -XX:+UseCompactObjectHeaders by default for Java 25, but I don't think that's any time especially soon, and I expect that are a lot of tests that care about the size of things so this test will be in good company then.

joegallo
joegallo approved these changes Sep 26, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@joegallo joegallo left a comment

Choose a reason for hiding this comment

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

🚢

@masseyke
Copy link
Copy Markdown
Member Author

masseyke commented Sep 26, 2025

This might give us a little trouble if/when we turn on -XX:+UseCompactObjectHeaders by default for Java 25, but I don't think that's any time especially soon, and I expect that are a lot of tests that care about the size of things so this test will be in good company then.

I don't think that what we compute will change -- it's pretty much all hard-coded values, not actual memory measurements. What we compute might get more wrong when we turn on -XX:+UseCompactObjectHeaders by default, but I think this test will continue to pass.

@masseyke masseyke merged commit 1e1bf28 into elastic:main Sep 26, 2025
34 checks passed
@masseyke masseyke deleted the testing-IndicesQueryCache-memory-stats branch September 26, 2025 20:17
szybia added a commit to szybia/elasticsearch that referenced this pull request Sep 29, 2025
@szybia
Merge remote-tracking branch 'upstream/main' into dls-stats-ordering
20a765d 
* upstream/main: (22 commits)
  Fix InternalCategorizationAggregationTests.testReduceRandom (elastic#135533)
  [DOCS] GeoIP processor: add clarification about using a reverse proxy endpoint (elastic#135534)
  Move `ProjectRoutingInfo` and related classes (elastic#135586)
  Refactor IndexAbstractionResolver (elastic#135587)
  Simplify returnLocalAll handling in ES|QL (elastic#135353)
  Reapply "Add an option to return early from an allocate call"  (elastic#135589)
  Mute org.elasticsearch.xpack.esql.qa.single_node.GenerativeIT test elastic#134407
  Mute org.elasticsearch.xpack.esql.heap_attack.HeapAttackIT testAggTooManyMvLongs elastic#135585
  Mute org.elasticsearch.multiproject.test.XpackWithMultipleProjectsClientYamlTestSuiteIT test {yaml=esql/60_usage/Basic ESQL usage output (telemetry) snapshot version} elastic#135579
  Mute org.elasticsearch.search.ccs.KnnVectorQueryBuilderCrossClusterSearchIT testKnnQueryWithCcsMinimizeRoundTripsFalse elastic#135573
  Mute org.elasticsearch.xpack.esql.inference.textembedding.TextEmbeddingOperatorTests testSimpleCircuitBreaking elastic#135569
  Add telemetry for `TS` command (elastic#135471)
  Mute org.elasticsearch.cluster.routing.allocation.decider.RestoreInProgressAllocationDeciderTests testCanAllocatePrimaryExistingInRestoreInProgress elastic#135566
  allocation: clarify RestoreInProgressAllocationDecider failure message (elastic#132307)
  [ES|QL] Register AggregateMetricDoubleLiteral (elastic#135054)
  Validate Logstash pipeline ID when creating. (elastic#135378)
  Migrate transport versions 8841050 through 8841041 (elastic#135555)
  Mute org.elasticsearch.search.ccs.SparseVectorQueryBuilderCrossClusterSearchIT testSparseVectorQueryWithCcsMinimizeRoundTripsFalse elastic#135559
  Mute org.elasticsearch.action.admin.cluster.stats.SearchUsageStatsTests testToXContent elastic#135558
  Testing indices query cache memory stats (elastic#135298)
  ...
@masseyke masseyke mentioned this pull request Nov 7, 2025
Merged
@masseyke
Copy link
Copy Markdown
Member Author

masseyke commented Nov 7, 2025

💚 All backports created successfully

Status Branch Result
✅ 9.1
✅ 8.19

Questions ?

Please refer to the Backport tool documentation

@masseyke masseyke mentioned this pull request Nov 7, 2025
Merged
masseyke added a commit to masseyke/elasticsearch that referenced this pull request Nov 7, 2025
@masseyke
Testing indices query cache memory stats (elastic#135298)
60d1090 
(cherry picked from commit 1e1bf28)
masseyke added a commit to masseyke/elasticsearch that referenced this pull request Nov 7, 2025
@masseyke
Testing indices query cache memory stats (elastic#135298)
1bf0769 
(cherry picked from commit 1e1bf28)
@masseyke
Copy link
Copy Markdown
Member Author

masseyke commented Nov 7, 2025

I'm backporting these changes to 8.19 and 9.1 so that I can cleanly backport #130857.

elasticsearchmachine pushed a commit that referenced this pull request Nov 7, 2025
@masseyke
Testing indices query cache memory stats (#135298) (#137754)
7e81740 
(cherry picked from commit 1e1bf28)
masseyke added a commit that referenced this pull request Nov 7, 2025
@masseyke
Testing indices query cache memory stats (#135298) (#137753)
408149c 
(cherry picked from commit 1e1bf28)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joegallo joegallo joegallo approved these changes

Assignees

No one assigned

Labels

:Data Management/Indices APIs DO NOT USE. Use ":Distributed/Indices APIs" or ":StorageEngine/Templates" instead. Team:Data Management (obsolete) DO NOT USE. This team no longer exists. >test Issues or PRs that are addressing/adding tests v8.19.8 v9.1.8 v9.2.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@masseyke @elasticsearchmachine @joegallo