Fix breaking change caused by opensearch core #1187

zane-neo · 2023-08-07T14:29:18Z

Description

Fix breaking changes caused by opensearch core change.

Issues Resolved

NA

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

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.

Signed-off-by: zane-neo <zaniu@amazon.com>

dhrubo-os

spotless is failing.

dhrubo-os · 2023-08-07T19:35:21Z

common/src/main/java/org/opensearch/ml/common/connector/Connector.java

@@ -93,7 +93,7 @@ static Connector fromStream(StreamInput in) throws IOException {
    }

    static Connector createConnector(XContentBuilder builder, String connectorProtocol) throws IOException {
-        String jsonStr = Strings.toString(builder);
+        String jsonStr = StringUtils.xContentBuilderToString(builder);


Can't we do : builder.toString(). Core also did the same: opensearch-project/OpenSearch@d090a7f

Yup thats the right way to do it.
The static method was refactored and moved to XContentBuilder

dhrubo-os · 2023-08-07T19:36:39Z

common/src/main/java/org/opensearch/ml/common/utils/StringUtils.java

@@ -82,4 +84,8 @@ public static Map<String, String> getParameterMap(Map<String, ?> parameterObjs)
        }
        return parameters;
    }
+
+    public static String xContentBuilderToString(XContentBuilder builder) {


I believe we don't need this method.

XContentBuilder already supports toString.

dhrubo-os · 2023-08-07T19:37:32Z

common/src/test/java/org/opensearch/ml/common/TestHelper.java

        if (wrapWithObject) {
            builder.startObject();
        }
        obj.toXContent(builder, ToXContent.EMPTY_PARAMS);
        if (wrapWithObject) {
            builder.endObject();
        }
-        String jsonStr = Strings.toString(builder);
+        String jsonStr = StringUtils.xContentBuilderToString(builder);


same: builder.toString() and all other places as well.

saratvemulapalli · 2023-08-07T20:25:01Z

common/src/test/java/org/opensearch/ml/common/TestHelper.java

@@ -26,15 +26,15 @@ public static <T> void testParse(ToXContentObject obj, Function<XContentParser,
    }

    public static <T> void testParse(ToXContentObject obj, Function<XContentParser, T> function, boolean wrapWithObject) throws IOException {
-        XContentBuilder builder = XContentFactory.contentBuilder(XContentType.JSON);
+        XContentBuilder builder = XContentFactory.jsonBuilder();


Suggested change

XContentBuilder builder = XContentFactory.jsonBuilder();

XContentBuilder builder = MediaTypeRegistry.contentBuilder(XContentType.JSON);

Just curious to know why we should prefer MediaTypeRegistry.contentBuilder(XContentType.JSON); over XContentFactory.jsonBuilder();? as both are doing the same.

public static XContentBuilder jsonBuilder() throws IOException { return MediaTypeRegistry.contentBuilder(XContentType.JSON); }

In this case it doesnt matter since we are using XContentType.JSON anyway. But incase we'd like to use a different MediaType (down the line in future) the plugin is abstracted out about the format OpenSearch engine uses. So I would prefer MediaType where ever possible.

With this change: opensearch-project/OpenSearch#9156 it would make XContentType.JSON generic.

I prefer to use the previous one because this is more like a syntactic sugar, and in ml-commons we support only json format for now, we can use MediaTypeRegistry in the future if we need to support different media type.

I prefer to use the previous one because this is more like a syntactic sugar,

I think this is fine for now. I was leaning toward eliminating XContentFactory altogether so as to not tempt a developer to use it in opensearch-core (and create the circular library dependency problem) but we already enforce no circular dependencies with the opensearch-core library. What we don't enforce are circular dependencies between other libraries.

The intent is for opensearch-core to be the "center of gravity". For example, downstream plugin/extensions takes a required dependency on core and optional dependencies on the other libraries (e.g., x-content, geo, telementry, aggregations, mappers, replication, storage). This keeps the downstream opensearch dependencies lean. Why do we care? Because if someone wants to run an OpenSearch as a function (e.g., lambda, or cloud function) package size limits affect bootstrap times, class loading, and overall customer cost. Developers can afford to be more memory wreckless if the project only runs on self hosted, but load times and package sizes matter in cloud native deployments. This is why I'm so adamant on breaking down the monolith and not building a bloated comprehensive opensearch-sdk.

saratvemulapalli · 2023-08-07T20:38:52Z

I didnt realize there are multiple PRs already trying to fix the build, I was chasing a bug #1179 and ended up with compile problems.
Feel free to use changes from this PR: #1189

Signed-off-by: zane-neo <zaniu@amazon.com>

codecov · 2023-08-08T02:48:04Z

Codecov Report

Merging #1187 (56c63fa) into 2.x (5b4bd51) will decrease coverage by 0.03%.
Report is 2 commits behind head on 2.x.
The diff coverage is 90.00%.

@@             Coverage Diff              @@
##                2.x    #1187      +/-   ##
============================================
- Coverage     78.94%   78.91%   -0.03%     
- Complexity     2118     2121       +3     
============================================
  Files           167      167              
  Lines          8633     8641       +8     
  Branches        869      870       +1     
============================================
+ Hits           6815     6819       +4     
- Misses         1425     1428       +3     
- Partials        393      394       +1

Flag	Coverage Δ
ml-commons	`78.91% <90.00%> (-0.03%)`	⬇️

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

Files Changed	Coverage Δ
.../opensearch/ml/action/handler/MLSearchHandler.java	`87.23% <ø> (ø)`
...n/model_group/TransportUpdateModelGroupAction.java	`77.69% <ø> (ø)`
.../cluster/MLCommonsClusterManagerEventListener.java	`80.00% <ø> (ø)`
...search/ml/helper/ConnectorAccessControlHelper.java	`76.54% <ø> (ø)`
...opensearch/ml/helper/ModelAccessControlHelper.java	`79.81% <ø> (ø)`
...a/org/opensearch/ml/model/MLModelGroupManager.java	`84.48% <ø> (ø)`
...main/java/org/opensearch/ml/task/MLTaskRunner.java	`97.72% <ø> (ø)`
...ch/ml/engine/algorithms/remote/ConnectorUtils.java	`70.83% <87.50%> (+2.08%)`	⬆️
...tion/connector/TransportCreateConnectorAction.java	`78.12% <100.00%> (ø)`
...java/org/opensearch/ml/utils/MLExceptionUtils.java	`86.36% <100.00%> (ø)`

... and 1 file with indirect coverage changes

dhrubo-os · 2023-08-08T16:22:16Z

I didnt realize there are multiple PRs already trying to fix the build, I was chasing a bug #1179 and ended up with compile problems.
Feel free to use changes from this PR: #1189

@saratvemulapalli this is not actually a bug. I will reply today to the issue in details today.

dhrubo-os · 2023-08-08T16:24:15Z

ava.lang.AssertionError
        at __randomizedtesting.SeedInfo.seed([5287C72090A89EF1:B9B8FAEEEA734B7B]:0)
        at org.junit.Assert.fail(Assert.java:87)
        at org.junit.Assert.assertTrue(Assert.java:42)
        at org.junit.Assert.assertTrue(Assert.java:53)
        at org.opensearch.ml.bwc.MLCommonsBackwardsCompatibilityIT.testBackwardsCompatibility(MLCommonsBackwardsCompatibilityIT.java:223)
  2> NOTE: leaving temporary files on disk at: /home/runner/work/ml-commons/ml-commons/plugin/build/testrun/mlCommonsBwcCluster#fullRestartClusterTask

@zane-neo BWC test is failing. Are we planning to fix this?

ylwu-amzn

LGTM. Thanks for fixing that breaking change. For BWC failure, that seems a different issue, can fix in a separate PR

Fix breaking change caused by opensearch core

b1cf15a

Signed-off-by: zane-neo <zaniu@amazon.com>

zane-neo had a problem deploying to ml-commons-cicd-env August 7, 2023 14:29 — with GitHub Actions Error

zane-neo had a problem deploying to ml-commons-cicd-env August 7, 2023 14:29 — with GitHub Actions Failure

zane-neo had a problem deploying to ml-commons-cicd-env August 7, 2023 14:29 — with GitHub Actions Error

dhrubo-os reviewed Aug 7, 2023

View reviewed changes

saratvemulapalli reviewed Aug 7, 2023

View reviewed changes

saratvemulapalli mentioned this pull request Aug 7, 2023

[2.x] Fixing breaking changes from core #1189

Closed

5 tasks

ylwu-amzn mentioned this pull request Aug 7, 2023

[RFC] Conversations and Generative AI in OpenSearch #1150

Closed

Use builder.toString() to get a builder string value and format code

56c63fa

Signed-off-by: zane-neo <zaniu@amazon.com>

zane-neo temporarily deployed to ml-commons-cicd-env August 8, 2023 02:30 — with GitHub Actions Inactive

ylwu-amzn approved these changes Aug 9, 2023

View reviewed changes

model-collapse approved these changes Aug 9, 2023

View reviewed changes

zane-neo merged commit d3d4122 into opensearch-project:2.x Aug 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix breaking change caused by opensearch core #1187

Fix breaking change caused by opensearch core #1187

zane-neo commented Aug 7, 2023 •

edited

Loading

dhrubo-os left a comment

dhrubo-os Aug 7, 2023

saratvemulapalli Aug 7, 2023

dhrubo-os Aug 7, 2023

saratvemulapalli Aug 7, 2023

dhrubo-os Aug 7, 2023 •

edited

Loading

saratvemulapalli Aug 7, 2023

dhrubo-os Aug 7, 2023

saratvemulapalli Aug 7, 2023 •

edited

Loading

saratvemulapalli Aug 7, 2023

zane-neo Aug 8, 2023

nknize Aug 8, 2023 •

edited

Loading

saratvemulapalli commented Aug 7, 2023

codecov bot commented Aug 8, 2023 •

edited

Loading

dhrubo-os commented Aug 8, 2023

dhrubo-os commented Aug 8, 2023

ylwu-amzn left a comment

	XContentBuilder builder = XContentFactory.jsonBuilder();
	XContentBuilder builder = MediaTypeRegistry.contentBuilder(XContentType.JSON);

Fix breaking change caused by opensearch core #1187

Fix breaking change caused by opensearch core #1187

Conversation

zane-neo commented Aug 7, 2023 • edited Loading

Description

Issues Resolved

Check List

dhrubo-os left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhrubo-os Aug 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saratvemulapalli Aug 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nknize Aug 8, 2023 • edited Loading

Choose a reason for hiding this comment

saratvemulapalli commented Aug 7, 2023

codecov bot commented Aug 8, 2023 • edited Loading

Codecov Report

dhrubo-os commented Aug 8, 2023

dhrubo-os commented Aug 8, 2023

ylwu-amzn left a comment

Choose a reason for hiding this comment

zane-neo commented Aug 7, 2023 •

edited

Loading

dhrubo-os Aug 7, 2023 •

edited

Loading

saratvemulapalli Aug 7, 2023 •

edited

Loading

nknize Aug 8, 2023 •

edited

Loading

codecov bot commented Aug 8, 2023 •

edited

Loading