changing MLSearchActionRequest to an instance subclass of SearchActionRequest #3459

dhrubo-os · 2025-01-29T20:54:35Z

In this PR, I'm making MLSearchActionRequest as a subclass of SearchActionRequest to follow the standard. Previously I wasn't quite extending the SearchActionRequest, as I had another SearchActionRequest object in the class and it was being read and written twice.

Description

[Describe what this change achieves]

Related Issues

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

Check List

New functionality includes testing.
New functionality has been documented.
API changes companion pull request created.
Commits are signed per the DCO using --signoff.
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.

b4sjoo · 2025-01-29T21:00:37Z

common/src/test/java/org/opensearch/ml/common/transport/search/MLSearchActionRequestTest.java

-import static org.junit.Assert.assertNotSame;
-import static org.junit.Assert.assertNull;
-import static org.junit.Assert.assertSame;
+import static org.junit.Assert.*;


Change wildcard import

addressed. That's in test file and we have a lot like this in ml-commons for test classes.

arjunkumargiri

Add details to github issues to provide description about why this PR is required, changes implemented as part of this PR and testing details.

arjunkumargiri · 2025-01-30T00:27:53Z

common/src/main/java/org/opensearch/ml/common/transport/search/MLSearchActionRequest.java

@@ -36,7 +35,7 @@ public class MLSearchActionRequest extends SearchRequest {
     */
    @Builder
    public MLSearchActionRequest(SearchRequest searchRequest, String tenantId) {
-        this.searchRequest = searchRequest;
+        super(searchRequest);


This constructs a new search request instead of reusing existing search request: https://github.com/opensearch-project/OpenSearch/blob/6f644e1c12de709b2136b999c8cb112a27c62100/server/src/main/java/org/opensearch/action/search/SearchRequest.java#L133

Would that be a memory concern?

Not sure how much memory concern will it be there. May be we can do something like:

// Use existing SearchRequest constructor to ensure minimal duplication super(searchRequest.indices(), searchRequest.source()); this.routing(searchRequest.routing()); this.preference(searchRequest.preference()); this.scroll(searchRequest.scroll()); this.allowPartialSearchResults(searchRequest.allowPartialSearchResults());

@msfroh do you have any input on this?

Given that the searchRequest.source() (and pretty much everything else) is copied by reference, the added memory is pretty negligible.

Doing the math, I count 14 object references (each at most 8 bytes, depending on CompressedOOPS, if I recall correctly), 2 booleans (which I think are 1 byte each -- I don't think they get packed smaller than that), and 2 ints (4 bytes each). So, I think a rough estimate would be 122 bytes?

msfroh

This approach seems fine to me, since you've already made MLSearchActionRequest extend SearchRequest.

It's also valid to make MLSearchActionRequest extend ActionRequest directly and hold a reference to SearchRequest. Functionally, there's not much difference either way.

msfroh · 2025-01-30T01:53:14Z

common/src/main/java/org/opensearch/ml/common/transport/search/MLSearchActionRequest.java

@@ -36,7 +35,7 @@ public class MLSearchActionRequest extends SearchRequest {
     */
    @Builder
    public MLSearchActionRequest(SearchRequest searchRequest, String tenantId) {
-        this.searchRequest = searchRequest;
+        super(searchRequest);


Given that the searchRequest.source() (and pretty much everything else) is copied by reference, the added memory is pretty negligible.

Doing the math, I count 14 object references (each at most 8 bytes, depending on CompressedOOPS, if I recall correctly), 2 booleans (which I think are 1 byte each -- I don't think they get packed smaller than that), and 2 ints (4 bytes each). So, I think a rough estimate would be 122 bytes?

dhrubo-os · 2025-01-30T02:10:12Z

No need add backport label, as this change has added to this PR in 2.x branch: https://github.com/opensearch-project/ml-commons/pull/3443/files

…nRequest Signed-off-by: Dhrubo Saha <dhrubo@amazon.com>

codecov · 2025-01-30T18:58:50Z

Codecov Report

Attention: Patch coverage is 77.77778% with 4 lines in your changes missing coverage. Please review.

Project coverage is 80.25%. Comparing base (c8ea5b1) to head (917577c).
Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
...rch/ml/action/tasks/SearchTaskTransportAction.java	25.00%	3 Missing ⚠️
...common/transport/search/MLSearchActionRequest.java	85.71%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #3459      +/-   ##
============================================
- Coverage     80.26%   80.25%   -0.02%     
- Complexity     6887     6902      +15     
============================================
  Files           608      610       +2     
  Lines         30008    30067      +59     
  Branches       3357     3367      +10     
============================================
+ Hits          24085    24129      +44     
- Misses         4480     4488       +8     
- Partials       1443     1450       +7

Flag	Coverage Δ
ml-commons	`80.25% <77.77%> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

dhrubo-os requested review from b4sjoo, mingshl, jngz-es, model-collapse, rbhavna, ylwu-amzn, zane-neo, Zhangxunmt, austintlee, HenryL27 and xinyual as code owners January 29, 2025 20:54

dhrubo-os had a problem deploying to ml-commons-cicd-env January 29, 2025 20:56 — with GitHub Actions Failure

b4sjoo reviewed Jan 29, 2025

View reviewed changes

dhrubo-os force-pushed the main branch from cdeac9a to 94267b7 Compare January 29, 2025 21:11

dhrubo-os had a problem deploying to ml-commons-cicd-env January 29, 2025 21:12 — with GitHub Actions Failure

dhrubo-os had a problem deploying to ml-commons-cicd-env January 29, 2025 22:01 — with GitHub Actions Failure

arjunkumargiri reviewed Jan 30, 2025

View reviewed changes

dhrubo-os had a problem deploying to ml-commons-cicd-env January 30, 2025 01:48 — with GitHub Actions Failure

msfroh approved these changes Jan 30, 2025

View reviewed changes

ylwu-amzn previously approved these changes Jan 30, 2025

View reviewed changes

Zhangxunmt previously approved these changes Jan 30, 2025

View reviewed changes

changing MLSearchActionRequest to an instance subclass of SearchActio…

917577c

…nRequest Signed-off-by: Dhrubo Saha <dhrubo@amazon.com>

dhrubo-os dismissed stale reviews from Zhangxunmt and ylwu-amzn via 917577c January 30, 2025 05:19

dhrubo-os force-pushed the main branch from 94267b7 to 917577c Compare January 30, 2025 05:19

dhrubo-os had a problem deploying to ml-commons-cicd-env January 30, 2025 05:21 — with GitHub Actions Failure

dhrubo-os temporarily deployed to ml-commons-cicd-env January 30, 2025 17:59 — with GitHub Actions Inactive

dhrubo-os temporarily deployed to ml-commons-cicd-env January 30, 2025 18:58 — with GitHub Actions Inactive

dhrubo-os temporarily deployed to ml-commons-cicd-env January 30, 2025 19:42 — with GitHub Actions Inactive

ylwu-amzn approved these changes Jan 30, 2025

View reviewed changes

jngz-es approved these changes Jan 30, 2025

View reviewed changes

dhrubo-os merged commit 0f871c0 into opensearch-project:main Jan 30, 2025
9 of 11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

changing MLSearchActionRequest to an instance subclass of SearchActionRequest #3459

changing MLSearchActionRequest to an instance subclass of SearchActionRequest #3459

dhrubo-os commented Jan 29, 2025 •

edited

Loading

b4sjoo Jan 29, 2025

dhrubo-os Jan 29, 2025

arjunkumargiri left a comment

arjunkumargiri Jan 30, 2025

dhrubo-os Jan 30, 2025

msfroh Jan 30, 2025

msfroh left a comment

msfroh Jan 30, 2025

dhrubo-os commented Jan 30, 2025

codecov bot commented Jan 30, 2025 •

edited

Loading

changing MLSearchActionRequest to an instance subclass of SearchActionRequest #3459

changing MLSearchActionRequest to an instance subclass of SearchActionRequest #3459

Conversation

dhrubo-os commented Jan 29, 2025 • edited Loading

Description

Related Issues

Check List

b4sjoo Jan 29, 2025

Choose a reason for hiding this comment

dhrubo-os Jan 29, 2025

Choose a reason for hiding this comment

arjunkumargiri left a comment

Choose a reason for hiding this comment

arjunkumargiri Jan 30, 2025

Choose a reason for hiding this comment

dhrubo-os Jan 30, 2025

Choose a reason for hiding this comment

msfroh Jan 30, 2025

Choose a reason for hiding this comment

msfroh left a comment

Choose a reason for hiding this comment

msfroh Jan 30, 2025

Choose a reason for hiding this comment

dhrubo-os commented Jan 30, 2025

codecov bot commented Jan 30, 2025 • edited Loading

Codecov Report

dhrubo-os commented Jan 29, 2025 •

edited

Loading

codecov bot commented Jan 30, 2025 •

edited

Loading