Replace the deprecated IndexReader APIs with new storedFields() & termVectors() by luyuncheng · Pull Request #7792 · opensearch-project/OpenSearch

luyuncheng · 2023-05-27T12:30:08Z

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.

Signed-off-by: luyuncheng <luyuncheng@bytedance.com>

2. Fixed some calling and Tests Signed-off-by: luyuncheng <luyuncheng@bytedance.com>

2. Fixed some calling and Tests 3. Spotless java Signed-off-by: luyuncheng <luyuncheng@bytedance.com>

github-actions · 2023-05-27T12:35:55Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/16419/
CommitID: 8603945
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2023-05-27T13:07:55Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❕
TEST FAILURES:

      1 org.opensearch.search.backpressure.SearchBackpressureIT.testSearchShardTaskCancellationWithHighCpu

URL: https://build.ci.opensearch.org/job/gradle-check/16420/
CommitID: c18cd7f
Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

codecov · 2023-05-27T18:08:34Z

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

gashutos

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.

server/src/main/java/org/opensearch/index/engine/TranslogLeafReader.java

Signed-off-by: luyuncheng <luyuncheng@bytedance.com>

luyuncheng · 2023-07-26T07:22:13Z

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 · 2023-07-26T07:40:11Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/20967/
CommitID: a29c62f
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2023-07-26T07:47:24Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/20968/
CommitID: 6532cf7
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

reta · 2023-07-26T13:18:22Z

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

…NOThreadLocal � Conflicts: � CHANGELOG.md

Signed-off-by: luyuncheng <luyuncheng@bytedance.com>

update CHANGELOG.md Signed-off-by: luyuncheng <luyuncheng@bytedance.com>

github-actions · 2023-07-26T14:54:13Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/20982/
CommitID: 949ebd4

github-actions · 2023-07-26T14:58:06Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/20983/
CommitID: 11caaaf

luyuncheng · 2023-07-26T15:07:45Z

conflicts :(, needs rebase with main

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

opensearch-trigger-bot · 2023-07-26T16:31:58Z

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 · 2023-07-26T16:34:01Z

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

luyuncheng · 2023-07-26T16:39:02Z

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

@reta OK, let me do it

…mVectors() (opensearch-project#7792) * 1. Remove calling deprecated document api Signed-off-by: luyuncheng <luyuncheng@bytedance.com> * 1. Remove calling deprecated document api 2. Fixed some calling and Tests Signed-off-by: luyuncheng <luyuncheng@bytedance.com> * 1. Remove calling deprecated document api 2. Fixed some calling and Tests 3. Spotless java Signed-off-by: luyuncheng <luyuncheng@bytedance.com> * add changelog Signed-off-by: luyuncheng <luyuncheng@bytedance.com> * add changelog Signed-off-by: luyuncheng <luyuncheng@bytedance.com> * merge main into branch Signed-off-by: luyuncheng <luyuncheng@bytedance.com> * merge main into branch update CHANGELOG.md Signed-off-by: luyuncheng <luyuncheng@bytedance.com> --------- Signed-off-by: luyuncheng <luyuncheng@bytedance.com>

…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 <cwperx@amazon.com>

…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 <cwperx@amazon.com> (cherry picked from commit 08d1734)

…mVectors() (opensearch-project#7792) * 1. Remove calling deprecated document api Signed-off-by: luyuncheng <luyuncheng@bytedance.com> * 1. Remove calling deprecated document api 2. Fixed some calling and Tests Signed-off-by: luyuncheng <luyuncheng@bytedance.com> * 1. Remove calling deprecated document api 2. Fixed some calling and Tests 3. Spotless java Signed-off-by: luyuncheng <luyuncheng@bytedance.com> * add changelog Signed-off-by: luyuncheng <luyuncheng@bytedance.com> * add changelog Signed-off-by: luyuncheng <luyuncheng@bytedance.com> * merge main into branch Signed-off-by: luyuncheng <luyuncheng@bytedance.com> * merge main into branch update CHANGELOG.md Signed-off-by: luyuncheng <luyuncheng@bytedance.com> --------- Signed-off-by: luyuncheng <luyuncheng@bytedance.com> Signed-off-by: Kaushal Kumar <ravi.kaushal97@gmail.com>

…mVectors() (opensearch-project#7792) * 1. Remove calling deprecated document api Signed-off-by: luyuncheng <luyuncheng@bytedance.com> * 1. Remove calling deprecated document api 2. Fixed some calling and Tests Signed-off-by: luyuncheng <luyuncheng@bytedance.com> * 1. Remove calling deprecated document api 2. Fixed some calling and Tests 3. Spotless java Signed-off-by: luyuncheng <luyuncheng@bytedance.com> * add changelog Signed-off-by: luyuncheng <luyuncheng@bytedance.com> * add changelog Signed-off-by: luyuncheng <luyuncheng@bytedance.com> * merge main into branch Signed-off-by: luyuncheng <luyuncheng@bytedance.com> * merge main into branch update CHANGELOG.md Signed-off-by: luyuncheng <luyuncheng@bytedance.com> --------- Signed-off-by: luyuncheng <luyuncheng@bytedance.com> Signed-off-by: Ivan Brusic <ivan.brusic@flocksafety.com>

…mVectors() (opensearch-project#7792) * 1. Remove calling deprecated document api Signed-off-by: luyuncheng <luyuncheng@bytedance.com> * 1. Remove calling deprecated document api 2. Fixed some calling and Tests Signed-off-by: luyuncheng <luyuncheng@bytedance.com> * 1. Remove calling deprecated document api 2. Fixed some calling and Tests 3. Spotless java Signed-off-by: luyuncheng <luyuncheng@bytedance.com> * add changelog Signed-off-by: luyuncheng <luyuncheng@bytedance.com> * add changelog Signed-off-by: luyuncheng <luyuncheng@bytedance.com> * merge main into branch Signed-off-by: luyuncheng <luyuncheng@bytedance.com> * merge main into branch update CHANGELOG.md Signed-off-by: luyuncheng <luyuncheng@bytedance.com> --------- Signed-off-by: luyuncheng <luyuncheng@bytedance.com> Signed-off-by: Shivansh Arora <hishiv@amazon.com>

luyuncheng requested review from Bukhtawar, CEHENKLE, Rishikesh1159, VachaShah, anasalkouz, andrross, dblock, dbwiddis, dreamer-89, gbbafna, kartg, kotwanikunal, mch2, nknize, owaiskazi19, reta, ryanbogan, saratvemulapalli, setiah, shwetathareja and tlfeng as code owners May 27, 2023 12:30

luyuncheng added 3 commits May 27, 2023 20:34

1. Remove calling deprecated document api

693733e

Signed-off-by: luyuncheng <luyuncheng@bytedance.com>

1. Remove calling deprecated document api

4db9da6

2. Fixed some calling and Tests Signed-off-by: luyuncheng <luyuncheng@bytedance.com>

1. Remove calling deprecated document api

c18cd7f

2. Fixed some calling and Tests 3. Spotless java Signed-off-by: luyuncheng <luyuncheng@bytedance.com>

luyuncheng force-pushed the NOThreadLocal branch from 8603945 to c18cd7f Compare May 27, 2023 12:35

luyuncheng changed the title ~~Reduce threadlocal when read store fields~~ Reduce Threadlocal and Memory when read store fields May 30, 2023

gashutos approved these changes Jun 28, 2023

View reviewed changes

server/src/main/java/org/opensearch/index/engine/TranslogLeafReader.java Show resolved Hide resolved

add changelog

a29c62f

Signed-off-by: luyuncheng <luyuncheng@bytedance.com>

luyuncheng requested review from sachinpkale and sohami as code owners July 26, 2023 07:18

add changelog

6532cf7

Signed-off-by: luyuncheng <luyuncheng@bytedance.com>

luyuncheng added 3 commits July 26, 2023 22:08

Merge branch 'main' of https://github.com/luyuncheng/OpenSearch into …

8806bd4

…NOThreadLocal � Conflicts: � CHANGELOG.md

merge main into branch

949ebd4

Signed-off-by: luyuncheng <luyuncheng@bytedance.com>

merge main into branch

11caaaf

update CHANGELOG.md Signed-off-by: luyuncheng <luyuncheng@bytedance.com>

reta approved these changes Jul 26, 2023

View reviewed changes

reta merged commit 4fd6877 into opensearch-project:main Jul 26, 2023

reta added the backport 2.x Backport to 2.x branch label Jul 26, 2023

luyuncheng mentioned this pull request Jul 26, 2023

[Backport 2.x] Replace the deprecated IndexReader APIs with new storedFields() & termVectors() #8893

Merged

This was referenced Jul 29, 2023

[Build Break] Fix build after Lucene upgrade and breaking XContentFactory changes opensearch-project/security#3069

Merged

Investigate CI Failures opensearch-project/security#3064

Closed

Conversation

luyuncheng commented May 27, 2023

Uh oh!

github-actions bot commented May 27, 2023

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions bot commented May 27, 2023

Gradle Check (Jenkins) Run Completed with:

Uh oh!

codecov bot commented May 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

gashutos left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

luyuncheng commented Jul 26, 2023

Uh oh!

github-actions bot commented Jul 26, 2023

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions bot commented Jul 26, 2023

Gradle Check (Jenkins) Run Completed with:

Uh oh!

reta commented Jul 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jul 26, 2023

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions bot commented Jul 26, 2023

Gradle Check (Jenkins) Run Completed with:

Uh oh!

luyuncheng commented Jul 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

opensearch-trigger-bot bot commented Jul 26, 2023

Uh oh!

reta commented Jul 26, 2023

Uh oh!

luyuncheng commented Jul 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented May 27, 2023 •

edited

Loading

gashutos left a comment •

edited

Loading

reta commented Jul 26, 2023 •

edited

Loading

luyuncheng commented Jul 26, 2023 •

edited

Loading

luyuncheng commented Jul 26, 2023 •

edited

Loading