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 a race condition in Derived Field parsing from search request #14445

Merged
merged 1 commit into from
Jun 24, 2024

Conversation

rishabhmaurya
Copy link
Contributor

@rishabhmaurya rishabhmaurya commented Jun 19, 2024

Description

flaky test was introduced because of a race condition in derived fields when defined within search request.
Since derived fields are created per QueryShardContext and QueryShardContext instance can be created multiple times for a search request, there is a race condition hitting here. Since we are reusing the parsing logic of mapper for derived fields defined in search requests too, it tries to modify the map passed to it where it tries to remove all keys it has parsed.

  1> Caused by: java.util.ConcurrentModificationException
  1>    at java.base/java.util.HashMap$HashIterator.remove(HashMap.java:1619)
  1>    at org.opensearch.index.mapper.ParametrizedFieldMapper$Builder.parse(ParametrizedFieldMapper.java:732)
  1>    at org.opensearch.index.mapper.ParametrizedFieldMapper$TypeParser.parse(ParametrizedFieldMapper.java:769)
  1>    at org.opensearch.index.mapper.ParametrizedFieldMapper$TypeParser.parse(ParametrizedFieldMapper.java:754)
  1>    at org.opensearch.index.mapper.ObjectMapper$TypeParser.parseDerived(ObjectMapper.java:384)
  1>    at org.opensearch.index.mapper.ObjectMapper$TypeParser.parseObjectOrDocumentTypeProperties(ObjectMapper.java:298)
  1>    at org.opensearch.index.mapper.RootObjectMapper$TypeParser.parse(RootObjectMapper.java:184)
  1>    at org.opensearch.index.mapper.DocumentMapperParser.parse(DocumentMapperParser.java:143)
  1>    at org.opensearch.index.mapper.DefaultDerivedFieldResolver.getAllDerivedFieldTypeFromObject(DefaultDerivedFieldResolver.java:194)
  1>    at org.opensearch.index.mapper.DefaultDerivedFieldResolver.initDerivedFieldTypes(DefaultDerivedFieldResolver.java:181)
  1>    at org.opensearch.index.mapper.DefaultDerivedFieldResolver.<init>(DefaultDerivedFieldResolver.java:64)
  1>    at org.opensearch.index.mapper.DefaultDerivedFieldResolver.<init>(DefaultDerivedFieldResolver.java:45)
  1>    at org.opensearch.index.mapper.DerivedFieldResolverFactory.createResolver(DerivedFieldResolverFactory.java:49)
  1>    at org.opensearch.search.SearchService.createSearchContext(SearchService.java:1121)
  1>    at 

Related Issues

Resolves #14444

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.

@github-actions github-actions bot added bug Something isn't working Search Search query, autocomplete ...etc labels Jun 19, 2024
Copy link
Contributor

✅ Gradle check result for a5e3f4b: SUCCESS

Copy link

codecov bot commented Jun 19, 2024

Codecov Report

Attention: Patch coverage is 47.36842% with 10 lines in your changes missing coverage. Please review.

Project coverage is 71.76%. Comparing base (b15cb0c) to head (2700d8d).
Report is 471 commits behind head on main.

Files Patch % Lines
...arch/index/mapper/DefaultDerivedFieldResolver.java 47.36% 8 Missing and 2 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main   #14445      +/-   ##
============================================
+ Coverage     71.42%   71.76%   +0.34%     
- Complexity    59978    62206    +2228     
============================================
  Files          4985     5125     +140     
  Lines        282275   292360   +10085     
  Branches      40946    42246    +1300     
============================================
+ Hits         201603   209814    +8211     
- Misses        63999    65261    +1262     
- Partials      16673    17285     +612     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rishabhmaurya rishabhmaurya added backport PRs or issues specific to backporting features or enhancments backport 2.x Backport to 2.x branch labels Jun 21, 2024
Copy link
Contributor

❌ Gradle check result for 1c6320e: 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 c77b334: 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 cdbfb0e: 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 e80ddba: 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?

@prudhvigodithi
Copy link
Contributor

Hey @rishabhmaurya based on the past issue here opensearch-project/opensearch-build#2292, force push will not work to trigger the gradle check with commit as github is not able to give the .pull_request.head.sha for the job
Error

+ set +x
Git clone: https://github.com/rishabhmaurya/OpenSearch.git with ref: e80ddbab5f324b449a2363f64493f02a16acbc4f
Cloning into 'search'...
fatal: reference is not a tree: e80ddbab5f324b449a2363f64493f02a16acbc4f

Adding @getsaurabh02

Copy link
Contributor

❌ Gradle check result for 0d8b2cb: 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 3bde396: SUCCESS

Copy link
Contributor

❌ Gradle check result for 9d517d7: 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: Rishabh Maurya <rishabhmaurya05@gmail.com>
Copy link
Contributor

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

Copy link
Contributor

✅ Gradle check result for cf64569: SUCCESS

@andrross andrross merged commit 212efd7 into opensearch-project:main Jun 24, 2024
30 of 31 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jun 24, 2024
…4445)

Signed-off-by: Rishabh Maurya <rishabhmaurya05@gmail.com>
(cherry picked from commit 212efd7)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
dblock pushed a commit that referenced this pull request Jun 25, 2024
…4445) (#14522)

(cherry picked from commit 212efd7)

Signed-off-by: Rishabh Maurya <rishabhmaurya05@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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport PRs or issues specific to backporting features or enhancments backport 2.x Backport to 2.x branch bug Something isn't working Search Search query, autocomplete ...etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] flaky test Test derived_field supported type using search definition
3 participants