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

Fix DerivedFieldQuery to support concurrent search. #15326

Merged
merged 8 commits into from
Aug 27, 2024

Conversation

mch2
Copy link
Member

@mch2 mch2 commented Aug 20, 2024

Description

This change updates DerivedFieldQuery to create a separate ValueFetcher instance per thread. The DerivedFieldValueFetcher is not thread safe in that it holds a ref to a compiled DerivedFieldScript that is created per thread. Each script also holds a SourceLookup object that is not thread safe.

Note there are no changes here specific to aggs with derived fields, that test already passes because we create a new ValueFetcher per leaf here.

Without this change these ITs fail with:

WARNING: Uncaught exception in thread: Thread[#170,opensearch[node_s1][search][T#1],5,TGRP-SimplePainlessIT]
java.lang.AssertionError: StoredFieldsReader are only supposed to be consumed in the thread in which they have been acquired. But was acquired in Thread[#171,opensearch[node_s1][index_searcher][T#1],5,TGRP-SimplePainlessIT] and consumed in Thread[#172,opensearch[node_s1][index_searcher][T#2],5,TGRP-SimplePainlessIT].
	at __randomizedtesting.SeedInfo.seed([77FDD1CE9C5EAC30]:0)
	at org.apache.lucene.tests.codecs.asserting.AssertingCodec.assertThread(AssertingCodec.java:44)
	at org.apache.lucene.tests.codecs.asserting.AssertingStoredFieldsFormat$AssertingStoredFieldsReader.document(AssertingStoredFieldsFormat.java:75)
	at org.opensearch.search.lookup.SourceLookup.loadSourceIfNeeded(SourceLookup.java:104)
	at org.opensearch.script.DerivedFieldScript.lambda$static$1(DerivedFieldScript.java:42)
	at org.opensearch.script.DynamicMap.get(DynamicMap.java:84)
	at org.opensearch.painless.PainlessScript$Script.execute(emit(params._source["field"]):12)
	at org.opensearch.index.mapper.DerivedFieldValueFetcher.fetchValuesInternal(DerivedFieldValueFetcher.java:54)
	at org.opensearch.index.mapper.DerivedFieldValueFetcher.getIndexableField(DerivedFieldValueFetcher.java:59)
	at org.opensearch.index.query.DerivedFieldQuery$1$1.matches(DerivedFieldQuery.java:99)

Related Issues

Resolves #15007

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 2e0d474: 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?

This change updates DerivedFieldQuery to create a separate ValueFetcher instance per thread.
The DerivedFieldValueFetcher is not thread safe in that it holds a ref to a compiled DerivedFieldScript that is
created per thread.  Each script also holds a SourceLookup object that is not thread safe.

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>
DerivedFieldQuery will accept a supplier for a valueFetcher rather than constructing it.
This ensures that the DerivedFieldType creating the query (obj or regular) passes the correct supplier func.

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>
Signed-off-by: Marc Handalian <marc.handalian@gmail.com>
Signed-off-by: Marc Handalian <marc.handalian@gmail.com>
Signed-off-by: Marc Handalian <marc.handalian@gmail.com>
Copy link
Contributor

❕ Gradle check result for 7cc1a6b: 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.

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>
Copy link
Contributor

❌ Gradle check result for 917ebd5: 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?

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>
Copy link
Contributor

✅ Gradle check result for 1520b44: SUCCESS

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>
Copy link
Contributor

❕ Gradle check result for 2373d0f: 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.

@mch2 mch2 merged commit c771bdd into opensearch-project:main Aug 27, 2024
33 of 34 checks passed
@mch2 mch2 deleted the 15007 branch August 27, 2024 22:06
opensearch-trigger-bot bot pushed a commit that referenced this pull request Aug 27, 2024
* Fix DerivedFieldQuery to support concurrent search.

This change updates DerivedFieldQuery to create a separate ValueFetcher instance per thread.
The DerivedFieldValueFetcher is not thread safe in that it holds a ref to a compiled DerivedFieldScript that is
created per thread.  Each script also holds a SourceLookup object that is not thread safe.

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>

* Fix broken cases relying on ObjectDerivedFieldValueFetcher.

DerivedFieldQuery will accept a supplier for a valueFetcher rather than constructing it.
This ensures that the DerivedFieldType creating the query (obj or regular) passes the correct supplier func.

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>

* remove unused clone method

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>

* Add changelog entry

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>

* add an extra test for DerivedFieldType multiPhraseQuery

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>

* more coverage

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>

* add tests for normalizedWildcard and phrase prefix

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>

---------

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>
(cherry picked from commit c771bdd)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
mch2 pushed a commit that referenced this pull request Aug 28, 2024
* Fix DerivedFieldQuery to support concurrent search.

This change updates DerivedFieldQuery to create a separate ValueFetcher instance per thread.
The DerivedFieldValueFetcher is not thread safe in that it holds a ref to a compiled DerivedFieldScript that is
created per thread.  Each script also holds a SourceLookup object that is not thread safe.



* Fix broken cases relying on ObjectDerivedFieldValueFetcher.

DerivedFieldQuery will accept a supplier for a valueFetcher rather than constructing it.
This ensures that the DerivedFieldType creating the query (obj or regular) passes the correct supplier func.



* remove unused clone method



* Add changelog entry



* add an extra test for DerivedFieldType multiPhraseQuery



* more coverage



* add tests for normalizedWildcard and phrase prefix



---------


(cherry picked from commit c771bdd)

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>
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>
akolarkunnu pushed a commit to akolarkunnu/OpenSearch that referenced this pull request Sep 10, 2024
…ct#15326)

* Fix DerivedFieldQuery to support concurrent search.

This change updates DerivedFieldQuery to create a separate ValueFetcher instance per thread.
The DerivedFieldValueFetcher is not thread safe in that it holds a ref to a compiled DerivedFieldScript that is
created per thread.  Each script also holds a SourceLookup object that is not thread safe.

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>

* Fix broken cases relying on ObjectDerivedFieldValueFetcher.

DerivedFieldQuery will accept a supplier for a valueFetcher rather than constructing it.
This ensures that the DerivedFieldType creating the query (obj or regular) passes the correct supplier func.

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>

* remove unused clone method

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>

* Add changelog entry

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>

* add an extra test for DerivedFieldType multiPhraseQuery

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>

* more coverage

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>

* add tests for normalizedWildcard and phrase prefix

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>

---------

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>
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 enhancement Enhancement or improvement to existing feature or request Search:Query Capabilities v2.17.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] Concurrent search support with Derived Fields
4 participants