Skip to content
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

Group field caps response by index mapping hash #83494

Merged
merged 9 commits into from
Feb 17, 2022
Merged

Group field caps response by index mapping hash #83494

merged 9 commits into from
Feb 17, 2022

Conversation

dnhatn
Copy link
Member

@dnhatn dnhatn commented Feb 4, 2022 •
edited
Loading

This commit utilizes the index mapping hash to share the fields-caps for indices with the same index mapping to reduce the memory usage and the size of transport messages.

Closes #78665
Closes #82879

@dnhatn dnhatn added the v8.2.0 label Feb 4, 2022
@dnhatn dnhatn force-pushed the field_caps_hash branch 2 times, most recently from 1eecca4 to 015e0e2 Compare February 10, 2022 01:27
@dnhatn dnhatn added :Search/Search Search-related issues that do not fall into other categories >enhancement labels Feb 10, 2022
@dnhatn dnhatn marked this pull request as ready for review February 10, 2022 03:54
@elasticmachine elasticmachine added the Team:Search Meta label for search team label Feb 10, 2022
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search (Team:Search)

@elasticsearchmachine
Copy link
Collaborator

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

@jtibshirani
Copy link
Contributor

Thanks @dnhatn ! I'll review soon. Am I understanding right that you don't think we should do anything beyond this for remote clusters (#78665)? For example, we will not try to fully reduce the response on the remote cluster, combining all mappings across indices.

@dnhatn
Copy link
Member Author

dnhatn commented Feb 11, 2022

I'll review soon.

Thank you, no worries! It's not urgent.

Am I understanding right that you don't think we should do anything beyond this for remote clusters (#78665)? For example, we will not try to fully reduce the response on the remote cluster, combining all mappings across indices.

That's correct. I think this is good enough now. We compact responses from each cluster and between them on the coordinating node.

@javanna javanna self-requested a review February 14, 2022 14:18
jtibshirani
jtibshirani reviewed Feb 14, 2022
View reviewed changes
Copy link
Contributor

@jtibshirani jtibshirani left a comment

Choose a reason for hiding this comment

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

I had one high-level question -- instead of what we do in this PR, would it be possible to detect duplicate mappings on the field caps coordinator, before sending all the node/ shard requests? Could this let us avoid sending duplicate requests in the first place, and keep the response merging logic simple?

@dnhatn
Copy link
Member Author

dnhatn commented Feb 14, 2022

Yes, we can apply your suggestion in RequestDispatcher for requests without indexFilter. However, we still need the deduplication optimization for cross-clusters requests or requests with indexFilter.

@dnhatn
Copy link
Member Author

dnhatn commented Feb 14, 2022

@jtibshirani I can make your suggestion in a follow-up.

@dnhatn dnhatn requested review from jtibshirani and removed request for original-brownbear February 14, 2022 20:52
@jtibshirani
Copy link
Contributor

I see, I forgot that this wouldn't work with indexFilter. I think it still works with CCS though, the coordinator on the remote cluster would just apply the same optimization? Sounds good to discuss in a follow-up.

@dnhatn
Copy link
Member Author

dnhatn commented Feb 14, 2022

I think it still works with CCS though, the coordinator on the remote cluster would just apply the same optimization?

Yes, that's correct, but we still have to deduplicate the response when sending it back to the local cluster.

jtibshirani
jtibshirani reviewed Feb 14, 2022
View reviewed changes
Copy link
Contributor

@jtibshirani jtibshirani left a comment

Choose a reason for hiding this comment

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

This is looking good to me, I left a few small comments. I also wondered if we could use the mapping hashes in TransportFieldCapabilitiesAction#merge to avoid merging merging in the same information twice.

server/src/main/java/org/elasticsearch/action/fieldcaps/TransportFieldCapabilitiesAction.java Show resolved Hide resolved
server/src/main/java/org/elasticsearch/action/fieldcaps/TransportFieldCapabilitiesAction.java Outdated Show resolved Hide resolved
server/src/main/java/org/elasticsearch/action/fieldcaps/FieldCapabilitiesIndexResponse.java Show resolved Hide resolved
server/src/main/java/org/elasticsearch/action/fieldcaps/TransportFieldCapabilitiesAction.java Show resolved Hide resolved
jtibshirani
jtibshirani approved these changes Feb 15, 2022
View reviewed changes
Copy link
Contributor

@jtibshirani jtibshirani left a comment

Choose a reason for hiding this comment

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

Sounds good to look into the merge optimization in a follow-up. This looks good to me. I know @javanna was hoping to take a look too when he had time.

server/src/main/java/org/elasticsearch/action/fieldcaps/FieldCapabilitiesIndexResponse.java Show resolved Hide resolved
@dnhatn
Copy link
Member Author

dnhatn commented Feb 17, 2022

@jtibshirani Thanks for your review.

@dnhatn dnhatn merged commit 69e898d into elastic:master Feb 17, 2022
@dnhatn dnhatn deleted the field_caps_hash branch February 17, 2022 15:19
weizijun added a commit to weizijun/elasticsearch that referenced this pull request Feb 18, 2022
@weizijun
Merge branch 'upstream/master' into rollup-index-improvement
4db3c09 
* upstream/master: (167 commits)
  Mute FrozenSearchableSnapshotsIntegTests#testCreateAndRestorePartialSearchableSnapshot
  Mute LdapSessionFactoryTests#testSslTrustIsReloaded
  Fix spotless violation from last commit
  Mute GeoGridTilerTestCase#testGeoGridSetValuesBoundingBoxes_UnboundedGeoShapeCellValues
  Small formatting clean up (elastic#84144)
  Always re-run Feature migrations which have encountered errors (elastic#83918)
  [DOCS] Clarify `orientation` usage for WKT and GeoJSON polygons (elastic#84025)
  Group field caps response by index mapping hash (elastic#83494)
  Shrink join queries in slow log (elastic#83914)
  TSDB: Reject the nested object fields that are configured time_series_dimension (elastic#83920)
  [DOCS] Remove note about partial response from Bulk API docs (elastic#84053)
  Allow regular data streams to be migrated to tsdb data streams. (elastic#83843)
  [DOCS] Fix `ignore_unavailable` parameter definition (elastic#84071)
  Make Metadata extend AbstractCollection (elastic#83791)
  Add API specs for OpenID Connect APIs
  Revert "Clean up for superuser role name references (elastic#83627)" (elastic#84096)
  Update Lucene analysis base url (elastic#84094)
  Avoid null threadContext in ResultDeduplicator (elastic#84093)
  Use static empty store files metadata (elastic#84034)
  Preserve context in snapshotDeletionListeners (elastic#84089)
  ...

# Conflicts:
#	x-pack/plugin/rollup/build.gradle
probakowski pushed a commit to probakowski/elasticsearch that referenced this pull request Feb 23, 2022
@dnhatn @probakowski
Group field caps response by index mapping hash (elastic#83494)
31ead1f 
This commit utilizes the index mapping hash to share the fields-caps for 
indices with the same index mapping to reduce the memory usage and the
size of transport messages.

Closes elastic#78665
Closes elastic#82879
@javanna
Copy link
Member

javanna commented Feb 24, 2022

I know @javanna was hoping to take a look too when he had time.

Took me a while, but I ended up diving into this :) I think it's a great improvement. One question I have is whether we think we have enough test coverage around the changes we made to the serialization here. I see a different path depending on whether the mapping hash is available, starting from the version that supports it, and I wonder if we should have more tests to cover what happens when nodes with multiple versions that may or may not provide the mapping hash are involved. The logic seems more complex than a simple "serialize the field or not depending on the stream version" hence I get anxious.

@dnhatn
Copy link
Member Author

dnhatn commented Feb 26, 2022

@javanna Thanks for looking. I think we have good coverage with the newly added test where we randomly generate index responses with and without mapping hashes and serialize with old and new versions.

@dnhatn
Copy link
Member Author

dnhatn commented Feb 28, 2022

@javanna I am working on some BWC tests for field caps. I will open a PR when they are ready.

@javanna javanna mentioned this pull request Feb 28, 2022
Closed
@javanna
Copy link
Member

javanna commented Feb 28, 2022

thanks @dnhatn for pointing me to the randomized test, that' definitely a good test. What causes confusion for me is that I don't see where we call setVersion on the stream to randomize the version. Do we also test cases where the mapping hash does not come back from an older node, yet we write the response to another node, which may or may not support the mapping hash? One more thing I do in these cases is test reading the response from an older version by storing the older format in base64 obtained from the previous version, rather than simulated from the code of the current version.

@dnhatn dnhatn mentioned this pull request Feb 28, 2022
Merged
@javanna javanna mentioned this pull request Mar 1, 2022
Closed
@dnhatn dnhatn mentioned this pull request Mar 3, 2022
Merged
dnhatn added a commit that referenced this pull request Mar 15, 2022
@dnhatn
Strengthen field-caps responses wire test (#84596)
fd60d2a 
Relates #83494
dnhatn added a commit that referenced this pull request Mar 15, 2022
@dnhatn
Add BWC test for field-caps (#84455)
273eedd 
Relates #83494
dnhatn added a commit to dnhatn/elasticsearch that referenced this pull request Mar 15, 2022
@dnhatn
Add BWC test for field-caps (elastic#84455)
3b3b1f9 
Relates elastic#83494
dnhatn added a commit to dnhatn/elasticsearch that referenced this pull request Mar 15, 2022
@dnhatn
Add BWC test for field-caps (elastic#84455)
22b1daf 
Relates elastic#83494
elasticsearchmachine pushed a commit that referenced this pull request Mar 15, 2022
@dnhatn
Add BWC test for field-caps (#84455) (#84989)
42c8300 
Relates #83494
elasticsearchmachine pushed a commit that referenced this pull request Mar 15, 2022
@dnhatn
[7.17] Add BWC test for field-caps (#84455) (#84990)
c831a61 
* Add BWC test for field-caps (#84455)

Relates #83494

* fix tests

* fix tests
@dnhatn dnhatn mentioned this pull request Jun 27, 2022
Merged
dnhatn added a commit that referenced this pull request Jun 29, 2022
@dnhatn
Reduce memory usage in field-caps responses (#88042)
95b23fc 
We have reduced the memory usage of field-caps requests targeting many 
indices in 8.2+ (see #83494). Unfortunately, we still receive OOM
reports in 7.17. I think we should push some contained improvements to
reduce the memory usage for those requests in 7.17. I have looked into
several options. This PR reduces the memory usage of field-caps
responses by replace HashMap with ArrayList for the field responses to
eliminate duplicated string names and internal nodes of Map.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jtibshirani jtibshirani jtibshirani approved these changes

@javanna javanna Awaiting requested review from javanna

Assignees
No one assigned
Labels
>enhancement :Search/Search Search-related issues that do not fall into other categories Team:Search Meta label for search team v8.2.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Merge field caps responses on each node? Field caps against many remote clusters consumes substantial heap
5 participants
@dnhatn @elasticmachine @elasticsearchmachine @jtibshirani @javanna