Update newQueryShardContext() to fix breaking changes

[harshavamsi]

Description

Fixes BWC for newQueryShardContext -- https://github.com/opensearch-project/OpenSearch/actions/runs/10710534586/job/29697453754?pr=15703

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

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.

[jainankitk]

@harshavamsi - Change looks good. I am curious if you noticed some tests failing?

[harshavamsi]

@harshavamsi - Change looks good. I am curious if you noticed some tests failing?

#15703 -- BWC tests were failing because I did not overload the method enough

[msfroh]

Yeah... They're a different kind of bwc check -- if a method is removed from a public API, the backport fails.

I missed that a method signature had changed and even thanked @harshavamsi prematurely for adding an overload. That thanks belongs on this PR instead.

[jainankitk]

@harshavamsi - Change looks good. I am curious if you noticed some tests failing?

#15703 -- BWC tests were failing because I did not overload the method enough

Thanks @harshavamsi. Interestingly, detect-breaking-changes check had not failed for this PR - #15538, which was also breaking.

@msfroh - Probably a gap we should address in the breaking change detector?

[jainankitk]

Yeah... They're a different kind of bwc check -- if a method is removed from a public API, the backport fails.

Yeah, I noticed this check failed even while changing namespace for ResourceType enum, since Java serialization would have failed. It did not have anything to do with OS serialization.

I missed that a method signature had changed and even thanked @harshavamsi prematurely for adding an overload. That thanks belongs on this PR instead.

Yeah, even I saw that and missed the other constructor myself. So, you're not the only one. That's why wondering if we can enhance the detect-breaking-change to catch other scenarios like abstract method as well.

[github-actions]

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

[harshavamsi]

Execution failed for task ':distribution:bwc:minor:buildBwcLinuxTar'.
> Building 2.17.0 didn't generate expected file /var/jenkins/workspace/gradle-check/search/distribution/bwc/minor/build/bwc/checkout-2.x/distribution/archives/linux-tar/build/distributions/opensearch-min-2.17.0-SNAPSHOT-linux-x64.tar.gz

huh how is it running bwc on 2.17?

I think this is failing on 2.17 because the original change has not been backported to 2.x yet. @msfroh what do you think we can do here?

[msfroh]

Can you cherry-pick the commit locally, and push it on your branch for the other change?

[jainankitk]

Can you cherry-pick the commit locally, and push it on your branch for the other change?

Seems to be the issue across all PRs - #15713 (comment). I think this has probably to do with bwc not setup for 2.17 yet!? Not sure if we need any manual step after bumping up the version to 2.17 in 2.x branch

[harshavamsi]

Can you cherry-pick the commit locally, and push it on your branch for the other change?

Seems to be the issue across all PRs - #15713 (comment). I think this has probably to do with bwc not setup for 2.17 yet!? Not sure if we need any manual step after bumping up the version to 2.17 in 2.x branch

#15712 -- is this because we already cut 2.17 branch and depend on this PR?

[msfroh]

We've had 2.17.0 builds all day. They've been coming off the 2.17 branch.

[jainankitk]

Do we need to bump up the bwc version in main to 2.18 after bumping to 2.18 for 2.x - #15680?

[jainankitk]

Okay, this PR needs to go in - #15712 before builds can start passing again! @bowenlan-amzn resolved similar issue #15053 (comment) when we released 2.16

[harshavamsi]

Okay, this PR needs to go in - #15712 before builds can start passing again! @bowenlan-amzn resolved similar issue #15053 (comment) when we released 2.16

#15053 (comment) -- based on this comment looks like the build manifest needs to be updated?

[msfroh]

Ahhh! What changed since I came home is that the 2.18 update on 2.x got merged. (We need to update 2.x before we update main.)

Now we need to update main, which maybe needs the manifest change first?

Or we need a 2.x Jenkins build to publish 2.18 artifacts. Then we can merge the 2.18 version change for main. Then everyone can rebase their PRs onto that.

[harshavamsi]

Ahhh! What changed since I came home is that the 2.18 update on 2.x got merged. (We need to update 2.x before we update main.)

Now we need to update main, which maybe needs the manifest change first?

Or we need a 2.x Jenkins build to publish 2.18 artifacts. Then we can merge the 2.18 version change for main. Then everyone can rebase their PRs onto that.

opensearch-project/opensearch-build#4908 (comment) -- I added a comment on the 2.17 release issue. Maybe we can automate the manifest update once a version is cut

[github-actions]

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

[github-actions]

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

[github-actions]

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

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.80%. Comparing base (729e40d) to head (c6246e6).
Report is 10 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #15710      +/-   ##
============================================
- Coverage     71.95%   71.80%   -0.16%     
+ Complexity    64192    64067     -125     
============================================
  Files          5270     5271       +1     
  Lines        300052   300167     +115     
  Branches      43368    43380      +12     
============================================
- Hits         215917   215530     -387     
- Misses        66442    66914     +472     
- Partials      17693    17723      +30     

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

[jainankitk]

@harshavamsi - I believe we don't need to backport this PR to 2.x/2.17, since the original PR never made there?

[harshavamsi]

@harshavamsi - I believe we don't need to backport this PR to 2.x/2.17, since the original PR never made there?

Right, I cherry picked this commit onto 2.x and 2.17 PRs so no need to backport