Skip to content

Testing indices query cache memory stats #135298

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 17 commits into from
Sep 26, 2025
Merged

Testing indices query cache memory stats #135298

merged 17 commits into from
Sep 26, 2025
+197 −12

Conversation

masseyke
Copy link
Member

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 APIs to create and manage indices and templates v9.2.0 labels Sep 23, 2025
@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine elasticsearchmachine added the Team:Data Management Meta label for data/management team label Sep 23, 2025
@joegallo
Copy link
Contributor

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
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
Contributor

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
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
Member Author

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)
  ...
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 APIs to create and manage indices and templates Team:Data Management Meta label for data/management team >test Issues or PRs that are addressing/adding tests v9.2.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@masseyke @elasticsearchmachine @joegallo