Skip to content

Create thread id to SourceLookup map #17927

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

Merged
merged 1 commit into from
Apr 28, 2025

Conversation

jed326
Copy link
Contributor

@jed326 jed326 commented Apr 14, 2025

Description

This PR includes changes to retrieve SourceLookup based on the current thread id. See #17743 (comment) for more details.

Testing

This PR includes new test coverage on the cache case, as well as the existing scripting tests passing.
More broadly we need to invest in better perf testing related to scripting: opensearch-project/opensearch-benchmark-workloads#362

Related Issues

Relates #17743

Check List

  • Functionality includes testing.
    - [ ] API changes companion pull request created, if applicable.
    - [ ] Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Copy link
Contributor

❌ Gradle check result for 501d8f9: FAILURE

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?

@sohami
Copy link
Contributor

sohami commented Apr 25, 2025

By adding new API we are not preventing any other usage of the existing API to fall into the same trap and the assumption here is that this FieldScript will only be used in the fetch phase. Instead as discussed, lets add the map with threadId to SourceLookup in the SearchLookup class to create SourceLookup per threadId. Also from the source method we will return the source lookup from the map to make the usage of SourceLookup consistent from within SearchLookup

Copy link
Contributor

❌ Gradle check result for eea1ced: FAILURE

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?

Copy link
Contributor

❌ Gradle check result for 9248b29: FAILURE

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?

@jed326 jed326 changed the title Use cached SourceLookup for FieldScript Create thread id to SourceLookup map Apr 26, 2025
Copy link
Contributor

❌ Gradle check result for c0f5565: FAILURE

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?

@jed326
Copy link
Contributor Author

jed326 commented Apr 26, 2025

Gradle check failing on this:

Cannot mock/spy class org.apache.lucene.index.LeafReaderContext
Mockito cannot mock/spy because :
 - final class
org.mockito.exceptions.base.MockitoException: 
Cannot mock/spy class org.apache.lucene.index.LeafReaderContext
Mockito cannot mock/spy because :
 - final class
	at __randomizedtesting.SeedInfo.seed([1791A3DB61DA55EA:558FD8B43BECCA70]:0)
	at org.opensearch.script.ScriptServiceTests.testScriptsUseCachedSourceLookup(ScriptServiceTests.java:197)

@jed326
Copy link
Contributor Author

jed326 commented Apr 26, 2025

I'm a little confused on this one as the mockito version we're on (5.16) is supposed to be able to mock final classes. Will go with an alternative approach for now while I look into that.

Copy link
Contributor

❕ Gradle check result for 42ae9a0: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

@github-project-automation github-project-automation bot moved this from Todo to In Progress in Concurrent Search Apr 28, 2025
@msfroh msfroh merged commit a06cf2c into opensearch-project:main Apr 28, 2025
32 checks passed
@github-project-automation github-project-automation bot moved this from In Progress to Done in Concurrent Search Apr 28, 2025
opensearch-trigger-bot bot pushed a commit that referenced this pull request Apr 28, 2025
Signed-off-by: Jay Deng <[email protected]>
(cherry picked from commit a06cf2c)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
andrross pushed a commit that referenced this pull request Apr 28, 2025
(cherry picked from commit a06cf2c)

Signed-off-by: Jay Deng <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
prudhvigodithi pushed a commit to prudhvigodithi/OpenSearch that referenced this pull request May 6, 2025
…pensearch-project#18107)

(cherry picked from commit a06cf2c)

Signed-off-by: Jay Deng <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Signed-off-by: Prudhvi Godithi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 3.0 skip-changelog v3.0.0 Issues and PRs related to version 3.0.0
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants