fix breaking changes in config index fields #2882

rbhavna · 2024-09-03T23:52:02Z

Description

fix breaking changes in config index fields

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.

Signed-off-by: Bhavana Ramaram <[email protected]>

ylwu-amzn · 2024-09-04T00:00:08Z

common/src/main/java/org/opensearch/ml/common/MLConfig.java

@@ -42,55 +42,93 @@ public class MLConfig implements ToXContentObject, Writeable {
    @Setter
    private String type;

+    @Setter
+    private String configType;


Add some comment like this is a new field to replace type field ?

ylwu-amzn · 2024-09-04T00:01:31Z

common/src/main/java/org/opensearch/ml/common/MLConfig.java

    }

    @Override
    public void writeTo(StreamOutput out) throws IOException {
        out.writeOptionalString(type);
+        out.writeOptionalString(configType);


Should we consider OS versions ?

Yeah I think so. There are going to be like completely new fields now. Not replacing the old ones

Signed-off-by: Bhavana Ramaram <[email protected]>

ylwu-amzn · 2024-09-04T00:24:37Z

common/src/main/java/org/opensearch/ml/common/MLConfig.java

+                out.writeBoolean(false);
+            }
+        }
+        out.writeOptionalInstant(lastUpdatedTime);


Why not add this new field to if block above?

sure, missed it somehow

Signed-off-by: Bhavana Ramaram <[email protected]>

lezzago · 2024-09-04T00:44:47Z

common/src/main/java/org/opensearch/ml/common/MLConfig.java

        if (type != null) {
-            builder.field(CONFIG_TYPE_FIELD, type);
+            builder.field(TYPE_FIELD, type);
+        }
+        if (configType != null) {
+            builder.field(CONFIG_TYPE_FIELD, configType);
        }
        if (configuration != null) {
-            builder.field(ML_CONFIGURATION_FIELD, configuration);
+            builder.field(CONFIGURATION_FIELD, configuration);
+        }
+        if (mlConfiguration != null) {
+            builder.field(ML_CONFIGURATION_FIELD, mlConfiguration);


What happens when ml_configuration and configuration are both populated. Same with config_type and config? Will this cause any issues in the plugin?

No there wouldn't be any issue but both will be shown in the response.

lezzago · 2024-09-04T00:47:43Z

common/src/main/java/org/opensearch/ml/common/MLConfig.java

    }

    public MLConfig(StreamInput input) throws IOException {
+        Version streamInputVersion = input.getVersion();


Shouldn't this be optional since it was not there in previous versions for upgrades?

lezzago · 2024-09-04T00:48:25Z

common/src/main/java/org/opensearch/ml/common/MLConfig.java

    }

    public MLConfig(StreamInput input) throws IOException {
+        Version streamInputVersion = input.getVersion();


Shouldn't this be optional since it was not there in previous versions for upgrades?

I didn't quite get it. This is only to ensure these new fields can be used only on domains starting from 2.15

Oh nevermind, I had made some confusion with the parse function. There should be no issues here.

lezzago · 2024-09-04T01:57:26Z

common/src/main/java/org/opensearch/ml/common/MLConfig.java

    }

    public MLConfig(StreamInput input) throws IOException {
+        Version streamInputVersion = input.getVersion();


Oh nevermind, I had made some confusion with the parse function. There should be no issues here.

* fix breaking changes in config index fields Signed-off-by: Bhavana Ramaram <[email protected]> (cherry picked from commit cc402b3)

opensearch-trigger-bot · 2024-09-04T03:17:46Z

The backport to 2.15 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.15 2.15
# Navigate to the new working tree
cd .worktrees/backport-2.15
# Create a new branch
git switch --create backport/backport-2882-to-2.15
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 cc402b30d317f0830f9baccdf57c1a663f54a538
# Push it to GitHub
git push --set-upstream origin backport/backport-2882-to-2.15
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.15

Then, create a pull request where the base branch is 2.15 and the compare/head branch is backport/backport-2882-to-2.15.

opensearch-trigger-bot · 2024-09-04T03:17:53Z

The backport to 2.16 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.16 2.16
# Navigate to the new working tree
cd .worktrees/backport-2.16
# Create a new branch
git switch --create backport/backport-2882-to-2.16
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 cc402b30d317f0830f9baccdf57c1a663f54a538
# Push it to GitHub
git push --set-upstream origin backport/backport-2882-to-2.16
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.16

Then, create a pull request where the base branch is 2.16 and the compare/head branch is backport/backport-2882-to-2.16.

* fix breaking changes in config index fields Signed-off-by: Bhavana Ramaram <[email protected]> (cherry picked from commit cc402b3)

ylwu-amzn · 2024-09-04T05:12:16Z

common/src/main/java/org/opensearch/ml/common/MLConfig.java

+            builder.field(TYPE_FIELD, type);
+        }
+        if (configType != null) {
+            builder.field(CONFIG_TYPE_FIELD, configType);


if (configType != null || type != null) { builder.field(TYPE_FIELD, configType == null ? type : configType); }

* fix breaking changes in config index fields Signed-off-by: Bhavana Ramaram <[email protected]> (cherry picked from commit cc402b3) Co-authored-by: Bhavana Ramaram <[email protected]>

rbhavna requested review from b4sjoo, dhrubo-os, jngz-es, model-collapse, ylwu-amzn, zane-neo, Zhangxunmt, austintlee, HenryL27, sam-herman and xinyual as code owners September 3, 2024 23:52

rbhavna temporarily deployed to ml-commons-cicd-env September 3, 2024 23:52 — with GitHub Actions Inactive

fix breaking changes in config index fields

02b0d56

Signed-off-by: Bhavana Ramaram <[email protected]>

rbhavna force-pushed the config_fix branch from 8d97cba to 02b0d56 Compare September 3, 2024 23:54

rbhavna temporarily deployed to ml-commons-cicd-env September 3, 2024 23:54 — with GitHub Actions Inactive

rbhavna temporarily deployed to ml-commons-cicd-env September 3, 2024 23:55 — with GitHub Actions Inactive

ylwu-amzn reviewed Sep 4, 2024

View reviewed changes

address comments

7aba355

Signed-off-by: Bhavana Ramaram <[email protected]>

rbhavna temporarily deployed to ml-commons-cicd-env September 4, 2024 00:21 — with GitHub Actions Inactive

rbhavna temporarily deployed to ml-commons-cicd-env September 4, 2024 00:22 — with GitHub Actions Inactive

ylwu-amzn reviewed Sep 4, 2024

View reviewed changes

address comments

9371c2e

Signed-off-by: Bhavana Ramaram <[email protected]>

rbhavna temporarily deployed to ml-commons-cicd-env September 4, 2024 00:29 — with GitHub Actions Inactive

rbhavna had a problem deploying to ml-commons-cicd-env September 4, 2024 00:29 — with GitHub Actions Failure

ylwu-amzn approved these changes Sep 4, 2024

View reviewed changes

lezzago reviewed Sep 4, 2024

View reviewed changes

lezzago approved these changes Sep 4, 2024

View reviewed changes

rbhavna had a problem deploying to ml-commons-cicd-env September 4, 2024 02:56 — with GitHub Actions Failure

Zhangxunmt approved these changes Sep 4, 2024

View reviewed changes

rbhavna merged commit cc402b3 into opensearch-project:main Sep 4, 2024
5 of 6 checks passed

rbhavna added backport 2.x backport 2.15 backport 2.16 backport 2.17 labels Sep 4, 2024

opensearch-trigger-bot bot pushed a commit that referenced this pull request Sep 4, 2024

fix breaking changes in config index fields (#2882)

04c7335

* fix breaking changes in config index fields Signed-off-by: Bhavana Ramaram <[email protected]> (cherry picked from commit cc402b3)

opensearch-trigger-bot bot mentioned this pull request Sep 4, 2024

[Backport 2.x] fix breaking changes in config index fields #2883

Merged

opensearch-trigger-bot bot pushed a commit that referenced this pull request Sep 4, 2024

fix breaking changes in config index fields (#2882)

803325f

* fix breaking changes in config index fields Signed-off-by: Bhavana Ramaram <[email protected]> (cherry picked from commit cc402b3)

opensearch-trigger-bot bot mentioned this pull request Sep 4, 2024

[Backport 2.17] fix breaking changes in config index fields #2884

Merged

ylwu-amzn reviewed Sep 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix breaking changes in config index fields #2882

fix breaking changes in config index fields #2882

rbhavna commented Sep 3, 2024

ylwu-amzn Sep 4, 2024

ylwu-amzn Sep 4, 2024

rbhavna Sep 4, 2024

ylwu-amzn Sep 4, 2024

rbhavna Sep 4, 2024

lezzago Sep 4, 2024

rbhavna Sep 4, 2024

lezzago Sep 4, 2024

lezzago Sep 4, 2024

rbhavna Sep 4, 2024

lezzago Sep 4, 2024

lezzago Sep 4, 2024

opensearch-trigger-bot bot commented Sep 4, 2024

opensearch-trigger-bot bot commented Sep 4, 2024

ylwu-amzn Sep 4, 2024 •

edited

Loading

fix breaking changes in config index fields #2882

fix breaking changes in config index fields #2882

Conversation

rbhavna commented Sep 3, 2024

Description

Related Issues

Check List

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

opensearch-trigger-bot bot commented Sep 4, 2024

opensearch-trigger-bot bot commented Sep 4, 2024

ylwu-amzn Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

ylwu-amzn Sep 4, 2024 •

edited

Loading