Skip to content

[Backport 2.x] Add verbose pipeline parameter to output each processor's execution #17097

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 3 commits into from
Jan 23, 2025

Conversation

junweid62
Copy link
Contributor

@junweid62 junweid62 commented Jan 23, 2025

Backport e15f712 from #16843.

Update version check to onOrAfter(Version.V_2_19_0)

Perform the BWC process:
1. Do this on main with onOrAfter(Version.V_3_0_0)). Get it merged.
2. You'll need a manual backport to 2.x, where you do onOrAfter(Version.V_2_19_0). Don't get it merged right away.
3. Before merging the backport to 2.x, open another PR on main to change it to onOrAfter(Version.V_2_19_0).
4. Merge the backport PR.
5. Merge the main version update PR.

Description

[Describe what this change achieves]

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.

…etails (opensearch-project#16843)

* Add verbose pipeline parameter to output each processor's execution details

Signed-off-by: Junwei Dai <[email protected]>

* add change log

Signed-off-by: Junwei Dai <[email protected]>

# Conflicts:
#	CHANGELOG.md

* Refactor ProcessorExecutionDetail to improve field handling

Signed-off-by: Junwei Dai <[email protected]>

* Fix ITtest Fail

Signed-off-by: Junwei Dai <[email protected]>

* Add more unit test

Signed-off-by: Junwei Dai <[email protected]>

* resolve comments

Signed-off-by: Junwei Dai <[email protected]>

* 1.add todo to change version.current
2.use exist xcontentUtil to read
3.move processor excution key to ProcessorExecutionDetail

Signed-off-by: Junwei Dai <[email protected]>

* refactor code

Signed-off-by: Junwei Dai <[email protected]>

* refactor code based on the comment

Signed-off-by: Junwei Dai <[email protected]>

* refactor code based on the comment

Signed-off-by: Junwei Dai <[email protected]>

* 1.add javadoc
2.refactor error message

Signed-off-by: Junwei Dai <[email protected]>

* change error message

Signed-off-by: Junwei Dai <[email protected]>

* 1.Added wrappers for tracking execution details of search processors.
2.Removed redundant logic for cleaner and simpler implementation.

Signed-off-by: Junwei Dai <[email protected]>

* change version to 3.0.0

Signed-off-by: Junwei Dai <[email protected]>

* fix unit test

Signed-off-by: Junwei Dai <[email protected]>

* fix unit test

Signed-off-by: Junwei Dai <[email protected]>

* addressed comments 1. removed unnecessary log

Signed-off-by: Junwei Dai <[email protected]>

* addressed comments

Signed-off-by: Junwei Dai <[email protected]>

* revise comment to opensearch.api

Signed-off-by: Junwei Dai <[email protected]>

* removed unused logger and comment

Signed-off-by: Junwei Dai <[email protected]>

* removed unnecessary try catch block. add more comment

Signed-off-by: Junwei Dai <[email protected]>

* addressed comments

Signed-off-by: Junwei Dai <[email protected]>

* remove wrong unit test

Signed-off-by: Junwei Dai <[email protected]>

---------

Signed-off-by: Junwei Dai <[email protected]>
Co-authored-by: Junwei Dai <[email protected]>
(cherry picked from commit e15f712)
Signed-off-by: Junwei Dai <[email protected]>
Copy link
Contributor

✅ Gradle check result for 46d14a5: SUCCESS

Copy link

codecov bot commented Jan 23, 2025

Codecov Report

Attention: Patch coverage is 75.56391% with 65 lines in your changes missing coverage. Please review.

Project coverage is 72.02%. Comparing base (bd885f7) to head (8dce9b4).
Report is 8 commits behind head on 2.x.

Files with missing lines Patch % Lines
...arch/search/pipeline/ProcessorExecutionDetail.java 79.56% 20 Missing and 8 partials ⚠️
...peline/TrackingSearchResponseProcessorWrapper.java 63.33% 11 Missing ⚠️
...ipeline/TrackingSearchRequestProcessorWrapper.java 66.66% 8 Missing ⚠️
...a/org/opensearch/action/search/SearchResponse.java 16.66% 4 Missing and 1 partial ⚠️
...ensearch/action/search/SearchResponseSections.java 75.00% 3 Missing ⚠️
...opensearch/search/builder/SearchSourceBuilder.java 82.35% 0 Missing and 3 partials ⚠️
...pensearch/rest/action/search/RestSearchAction.java 0.00% 1 Missing and 1 partial ⚠️
...search/search/internal/InternalSearchResponse.java 85.71% 2 Missing ⚠️
.../java/org/opensearch/search/pipeline/Pipeline.java 75.00% 0 Missing and 2 partials ⚠️
...g/opensearch/search/pipeline/PipelinedRequest.java 83.33% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##                2.x   #17097      +/-   ##
============================================
+ Coverage     71.84%   72.02%   +0.18%     
- Complexity    65595    65738     +143     
============================================
  Files          5318     5322       +4     
  Lines        305846   306191     +345     
  Branches      44602    44650      +48     
============================================
+ Hits         219739   220547     +808     
+ Misses        67752    67232     -520     
- Partials      18355    18412      +57     

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

@junweid62
Copy link
Contributor Author

Hi @msfroh, could you please take a look at this PR when you have a moment? I performed BWC trick that mentioned at
#16843

@msfroh
Copy link
Contributor

msfroh commented Jan 23, 2025

@junweid62 -- can you add an override constructor to SearchResponse?

The "Detect Breaking Changes" action is complaining about the fact that you modified an existing constructor rather than adding an override:

---! REMOVED CONSTRUCTOR: PUBLIC(-) SearchResponseSections(org.opensearch.search.SearchHits, org.opensearch.search.aggregations.Aggregations, org.opensearch.search.suggest.Suggest, boolean, java.lang.Boolean, org.opensearch.search.profile.SearchProfileShardResults, int, java.util.List<org.opensearch.search.SearchExtBuilder

You need to add a constructor that has the old signature that delegates to this(..., Collections.emptyList()) (for the empty set of processor details).

Copy link
Contributor

✅ Gradle check result for 8dce9b4: SUCCESS

@owaiskazi19
Copy link
Member

@msfroh @junweid62 we can add this constructor back on main as well. Since we don't run the breaking change workflow on main we might have not detected this before but can be a breaking change since we are releasing 3.0 soon.
Can be added in #17098

@owaiskazi19 owaiskazi19 merged commit 421f211 into opensearch-project:2.x Jan 23, 2025
45 checks passed
@msfroh
Copy link
Contributor

msfroh commented Jan 23, 2025

we can add this constructor back on main as well.

We don't need to. The 3.0 release is allowed to introduce API changes.

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.

3 participants