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.

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

Replace the deprecated IndexReader APIs with new storedFields() & termVectors() #7792

Merged
merged 8 commits into from
Jul 26, 2023

Conversation

luyuncheng
Copy link
Contributor

i try to use OpenSearch in production. and in our environment, we have tons of segments, tons of threads, tons of data.

i see there is deprecated API to retrieve document. These API would create many ThreadLocal StoreFields and make jvm can not GC. you can see the example in LUCENE#11987.

so i PR the code to replace the deprecated api with new storefields & termVectors API and reduced threadlocal creation.

2. Fixed some calling and Tests

Signed-off-by: luyuncheng <[email protected]>
2. Fixed some calling and Tests
3. Spotless java

Signed-off-by: luyuncheng <[email protected]>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.search.backpressure.SearchBackpressureIT.testSearchShardTaskCancellationWithHighCpu

@codecov
Copy link

codecov bot commented May 27, 2023

Codecov Report

Merging #7792 (11caaaf) into main (e1a4125) will decrease coverage by 0.03%.
The diff coverage is 57.14%.

@@             Coverage Diff              @@
##               main    #7792      +/-   ##
============================================
- Coverage     71.08%   71.05%   -0.03%     
+ Complexity    57237    57222      -15     
============================================
  Files          4758     4758              
  Lines        269844   269851       +7     
  Branches      39480    39481       +1     
============================================
- Hits         191819   191750      -69     
- Misses        61865    61969     +104     
+ Partials      16160    16132      -28     
Files Changed Coverage Δ
...opensearch/common/lucene/search/XMoreLikeThis.java 36.61% <0.00%> (-0.30%) ⬇️
...earch/fetch/subphase/highlight/HighlightUtils.java 40.00% <0.00%> (ø)
...rg/opensearch/index/engine/TranslogLeafReader.java 38.00% <50.00%> (+0.50%) ⬆️
...ensearch/index/termvectors/TermVectorsService.java 17.58% <50.00%> (+0.36%) ⬆️
...ensearch/gateway/PersistedClusterStateService.java 88.41% <100.00%> (ø)
...va/org/opensearch/index/engine/InternalEngine.java 74.58% <100.00%> (-0.69%) ⬇️
...opensearch/index/engine/LuceneChangesSnapshot.java 85.41% <100.00%> (ø)
...java/org/opensearch/index/get/ShardGetService.java 60.89% <100.00%> (+0.64%) ⬆️
...rg/opensearch/index/shard/ShardSplittingQuery.java 74.04% <100.00%> (ø)
...n/java/org/opensearch/search/fetch/FetchPhase.java 38.95% <100.00%> (ø)
... and 2 more

... and 447 files with indirect coverage changes

@luyuncheng luyuncheng changed the title Reduce threadlocal when read store fields Reduce Threadlocal and Memory when read store fields May 30, 2023
Copy link
Contributor

@gashutos gashutos left a comment

Choose a reason for hiding this comment

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

Thanks @luyuncheng just a minor comment but looks good to me !
There are no other places we use these 2 deprecated APIs right ?

Also tagging @dblock / @reta for additional reviews.

Signed-off-by: luyuncheng <[email protected]>
Signed-off-by: luyuncheng <[email protected]>
@luyuncheng
Copy link
Contributor Author

I think the description of the change is a bit misleading, may be change it to:

We also need a CHANGELOG entry please (see please CHANGELOG.md), thank you

++ Updated
@reta thanks for the review

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@reta
Copy link
Collaborator

reta commented Jul 26, 2023

Apologies @luyuncheng , conflicts :(, needs rebase with main

…NOThreadLocal

� Conflicts:
�	CHANGELOG.md
Signed-off-by: luyuncheng <[email protected]>
update CHANGELOG.md

Signed-off-by: luyuncheng <[email protected]>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@luyuncheng
Copy link
Contributor Author

luyuncheng commented Jul 26, 2023

conflicts :(, needs rebase with main

@reta Thanks. i sync upstream master into this branch, fixed conflicts. and run precommit locally seems success.

@reta reta merged commit 4fd6877 into opensearch-project:main Jul 26, 2023
8 of 9 checks passed
@reta reta added the backport 2.x Backport to 2.x branch label Jul 26, 2023
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-7792-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 4fd6877eef7c9cb0b48268a7505506aeb33278a2
# Push it to GitHub
git push --set-upstream origin backport/backport-7792-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-7792-to-2.x.

@reta
Copy link
Collaborator

reta commented Jul 26, 2023

@luyuncheng sadly automation failed, could you please send manual backport to 2.x branch? thank you

@luyuncheng
Copy link
Contributor Author

luyuncheng commented Jul 26, 2023

could you please send manual backport to 2.x branch? thank you

@reta OK, let me do it

sarthakaggarwal97 pushed a commit to sarthakaggarwal97/OpenSearch that referenced this pull request Jul 27, 2023
…mVectors() (opensearch-project#7792)

* 1. Remove calling deprecated document api

Signed-off-by: luyuncheng <[email protected]>

* 1. Remove calling deprecated document api
2. Fixed some calling and Tests

Signed-off-by: luyuncheng <[email protected]>

* 1. Remove calling deprecated document api
2. Fixed some calling and Tests
3. Spotless java

Signed-off-by: luyuncheng <[email protected]>

* add changelog

Signed-off-by: luyuncheng <[email protected]>

* add changelog

Signed-off-by: luyuncheng <[email protected]>

* merge main into branch

Signed-off-by: luyuncheng <[email protected]>

* merge main into branch
update CHANGELOG.md

Signed-off-by: luyuncheng <[email protected]>

---------

Signed-off-by: luyuncheng <[email protected]>
baba-devv pushed a commit to baba-devv/OpenSearch that referenced this pull request Jul 29, 2023
…mVectors() (opensearch-project#7792)

* 1. Remove calling deprecated document api

Signed-off-by: luyuncheng <[email protected]>

* 1. Remove calling deprecated document api
2. Fixed some calling and Tests

Signed-off-by: luyuncheng <[email protected]>

* 1. Remove calling deprecated document api
2. Fixed some calling and Tests
3. Spotless java

Signed-off-by: luyuncheng <[email protected]>

* add changelog

Signed-off-by: luyuncheng <[email protected]>

* add changelog

Signed-off-by: luyuncheng <[email protected]>

* merge main into branch

Signed-off-by: luyuncheng <[email protected]>

* merge main into branch
update CHANGELOG.md

Signed-off-by: luyuncheng <[email protected]>

---------

Signed-off-by: luyuncheng <[email protected]>
willyborankin pushed a commit to opensearch-project/security that referenced this pull request Jul 31, 2023
…3069)

There are multiple PRs in core affecting the security plugin that the
security plugin needs to adapt to.

- opensearch-project/OpenSearch#7792
- opensearch-project/OpenSearch#8826
- opensearch-project/OpenSearch#8668

I am opening a Draft PR that includes a fix for the Lucene-related test
failures which was caused by
opensearch-project/OpenSearch#7792

Resolves: #3064

Signed-off-by: Craig Perkins <[email protected]>
cwperks added a commit to cwperks/security that referenced this pull request Jul 31, 2023
…pensearch-project#3069)

There are multiple PRs in core affecting the security plugin that the
security plugin needs to adapt to.

- opensearch-project/OpenSearch#7792
- opensearch-project/OpenSearch#8826
- opensearch-project/OpenSearch#8668

I am opening a Draft PR that includes a fix for the Lucene-related test
failures which was caused by
opensearch-project/OpenSearch#7792

Resolves: opensearch-project#3064

Signed-off-by: Craig Perkins <[email protected]>
(cherry picked from commit 08d1734)
kaushalmahi12 pushed a commit to kaushalmahi12/OpenSearch that referenced this pull request Sep 12, 2023
…mVectors() (opensearch-project#7792)

* 1. Remove calling deprecated document api

Signed-off-by: luyuncheng <[email protected]>

* 1. Remove calling deprecated document api
2. Fixed some calling and Tests

Signed-off-by: luyuncheng <[email protected]>

* 1. Remove calling deprecated document api
2. Fixed some calling and Tests
3. Spotless java

Signed-off-by: luyuncheng <[email protected]>

* add changelog

Signed-off-by: luyuncheng <[email protected]>

* add changelog

Signed-off-by: luyuncheng <[email protected]>

* merge main into branch

Signed-off-by: luyuncheng <[email protected]>

* merge main into branch
update CHANGELOG.md

Signed-off-by: luyuncheng <[email protected]>

---------

Signed-off-by: luyuncheng <[email protected]>
Signed-off-by: Kaushal Kumar <[email protected]>
brusic pushed a commit to brusic/OpenSearch that referenced this pull request Sep 25, 2023
…mVectors() (opensearch-project#7792)

* 1. Remove calling deprecated document api

Signed-off-by: luyuncheng <[email protected]>

* 1. Remove calling deprecated document api
2. Fixed some calling and Tests

Signed-off-by: luyuncheng <[email protected]>

* 1. Remove calling deprecated document api
2. Fixed some calling and Tests
3. Spotless java

Signed-off-by: luyuncheng <[email protected]>

* add changelog

Signed-off-by: luyuncheng <[email protected]>

* add changelog

Signed-off-by: luyuncheng <[email protected]>

* merge main into branch

Signed-off-by: luyuncheng <[email protected]>

* merge main into branch
update CHANGELOG.md

Signed-off-by: luyuncheng <[email protected]>

---------

Signed-off-by: luyuncheng <[email protected]>
Signed-off-by: Ivan Brusic <[email protected]>
shiv0408 pushed a commit to Gaurav614/OpenSearch that referenced this pull request Apr 25, 2024
…mVectors() (opensearch-project#7792)

* 1. Remove calling deprecated document api

Signed-off-by: luyuncheng <[email protected]>

* 1. Remove calling deprecated document api
2. Fixed some calling and Tests

Signed-off-by: luyuncheng <[email protected]>

* 1. Remove calling deprecated document api
2. Fixed some calling and Tests
3. Spotless java

Signed-off-by: luyuncheng <[email protected]>

* add changelog

Signed-off-by: luyuncheng <[email protected]>

* add changelog

Signed-off-by: luyuncheng <[email protected]>

* merge main into branch

Signed-off-by: luyuncheng <[email protected]>

* merge main into branch
update CHANGELOG.md

Signed-off-by: luyuncheng <[email protected]>

---------

Signed-off-by: luyuncheng <[email protected]>
Signed-off-by: Shivansh Arora <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport to 2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants