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

[Backport 1.x] Fix class_cast_exception when passing int to _version and other metadata fields in ingest simulate API #10479

Conversation

gaobinlong
Copy link
Collaborator

@gaobinlong gaobinlong commented Oct 7, 2023

Description

Backport #10101 into 1.x

Related Issues

#10097

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog)
  • Public documentation issue/PR created

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.

…ata fields in ingest simulate API (opensearch-project#10101)

* Fix class_cast_exception when passing int to _version and other metadata fields in ingest simulate API

Signed-off-by: Gao Binlong <gbinlong@amazon.com>

* modify change log

Signed-off-by: Gao Binlong <gbinlong@amazon.com>

* Add more tests

Signed-off-by: Gao Binlong <gbinlong@amazon.com>

---------

Signed-off-by: Gao Binlong <gbinlong@amazon.com>
Signed-off-by: Daniel (dB.) Doubrovkine <dblock@amazon.com>
Co-authored-by: Daniel (dB.) Doubrovkine <dblock@amazon.com>
(cherry picked from commit fdaa438)
@github-actions
Copy link
Contributor

github-actions bot commented Oct 7, 2023

Gradle Check (Jenkins) Run Completed with:

Signed-off-by: Gao Binlong <gbinlong@amazon.com>
@github-actions
Copy link
Contributor

github-actions bot commented Oct 7, 2023

Gradle Check (Jenkins) Run Completed with:

Signed-off-by: Gao Binlong <gbinlong@amazon.com>
@github-actions
Copy link
Contributor

github-actions bot commented Oct 7, 2023

Gradle Check (Jenkins) Run Completed with:

@codecov
Copy link

codecov bot commented Oct 7, 2023

Codecov Report

Merging #10479 (33190d7) into 1.x (4cfcedd) will increase coverage by 0.02%.
The diff coverage is 92.30%.

@@             Coverage Diff              @@
##                1.x   #10479      +/-   ##
============================================
+ Coverage     77.55%   77.57%   +0.02%     
- Complexity    58792    58818      +26     
============================================
  Files          4223     4223              
  Lines        253563   253571       +8     
  Branches      38705    38708       +3     
============================================
+ Hits         196659   196716      +57     
+ Misses        40932    40862      -70     
- Partials      15972    15993      +21     
Files Coverage Δ
...nsearch/action/ingest/SimulatePipelineRequest.java 96.70% <92.30%> (-0.89%) ⬇️

... and 452 files with indirect coverage changes

@gaobinlong
Copy link
Collaborator Author

There's a test failure which only happens in this backport 1.x PR, but not exist in the PRs of main branch and 2.x branch, after diving deep into it, I found that's because some test methods never run in both main and 2.x branch, but run normally in 1.x branch, seems relates to the previous PR #2491 which changed the private method innerTestParseWithProvidedPipeline to public but maybe because the method name doesn't start with test so this method never runs, and also relates to PR #2239 which changed the private method innerTestParseUsingPipelineStore to public, renamed the method name but didn't remove the input parameter, so the new method testParseUsingPipelineStore also never runs.

Here are the detail of the related changes:

I've fixed the test failure of this pr, but because the failed test method never runs in both main branch and 2.x branch, and the pr of main branch has been merged yet, so I'll open a new issue to talk about it and fix it for main branch and 2.x branch.

@dblock could you help to take a look at this PR and the issue I've mentioned above.

@dblock
Copy link
Member

dblock commented Oct 10, 2023

@gaobinlong I merged #10496 and labeled it to backport to 2.x. Do the rightful please, just lmk what needs to be merged.

AFAIK we usually only merge security fixes to 1.x - but I imagine you need it? Not sure what the protocol is, maybe @bbarani or @CEHENKLE can take a look - does this get merge to 1.x?

@gaobinlong
Copy link
Collaborator Author

gaobinlong commented Oct 11, 2023

@dblock , thanks for your help, we don't need it in 1.x actually, that's because I see the original PR was labeled with 1.x and the automatic backport failed, so I put up this PR, maybe we can close this PR if there are some misunderstanding, but it really helps us to find a bug that some test methods never run. Thanks.

@dblock dblock closed this Oct 13, 2023
@dblock
Copy link
Member

dblock commented Oct 13, 2023

I closed without merging. Thanks for your good work!

@gaobinlong gaobinlong deleted the backport/backport-10101-to-1.x branch June 19, 2024 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants