Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Remove opendistro settings and endpoints #3326

Merged
merged 3 commits into from
Feb 20, 2025
Merged

Remove opendistro settings and endpoints #3326

merged 3 commits into from
Feb 20, 2025

Conversation

noCharger
Copy link
Collaborator

@noCharger noCharger commented Feb 17, 2025 •
edited
Loading

Description

Related Issues

Check List

  • 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.

@noCharger
Remove opendistro settings
50239aa 
Signed-off-by: Louis Chu <clingzhi@amazon.com>
@noCharger
Remove OpenDistro endpoints
30586c5 
Signed-off-by: Louis Chu <clingzhi@amazon.com>
@noCharger noCharger changed the title Remove opendistro settings Remove opendistro settings and endpoints Feb 18, 2025
@penghuo penghuo added the v3.0.0 label Feb 18, 2025
penghuo
penghuo previously approved these changes Feb 18, 2025
View reviewed changes
Copy link
Collaborator

@penghuo penghuo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx!

@noCharger
Copy link
Collaborator Author 

SQLBackwardsCompatibilityIT > testBackwardsCompatibility FAILED
    org.junit.ComparisonFailure: expected:<...t":{},"persistent":{["plugins.sql.cursor.keep_alive":"7m"]}}> but was:<...t":{},"persistent":{[]}}>
        at __randomizedtesting.SeedInfo.seed([33BCEBCEDDBE50B3:D883D600A7658539]:0)
        at org.junit.Assert.assertEquals(Assert.java:117)
        at org.junit.Assert.assertEquals(Assert.java:146)
        at org.opensearch.sql.bwc.SQLBackwardsCompatibilityIT.verifySQLSettings(SQLBackwardsCompatibilityIT.java:134)
        at org.opensearch.sql.bwc.SQLBackwardsCompatibilityIT.testBackwardsCompatibility(SQLBackwardsCompatibilityIT.java:97)
  1> [2025-02-18T02:29:53,909][INFO ][o.o.s.b.SQLBackwardsCompatibilityIT] [testBackwardsCompatibility] before test
  1> [2025-02-18T02:29:54,088][INFO ][o.o.s.b.SQLBackwardsCompatibilityIT] [testBackwardsCompatibility] initializing REST clients against [http://[::1]:42701, http://127.0.0.1:39011, http://[::1]:[423](https://github.com/opensearch-project/sql/actions/runs/13380702669/job/37368667914?pr=3326#step:6:424)69, http://127.0.0.1:37635, http://[::1]:33825, http://127.0.0.1:33885]
  1> [2025-02-18T02:29:54,652][INFO ][o.o.s.b.SQLBackwardsCompatibilityIT] [testBackwardsCompatibility] after test

Tests with failures:
 - org.opensearch.sql.bwc.SQLBackwardsCompatibilityIT.testBackwardsCompatibility

will do a sanity test before merging

@noCharger
Fix bwc IT gracefully
a4c4e8a 
Signed-off-by: Louis Chu <clingzhi@amazon.com>
@noCharger noCharger force-pushed the remove-opendistro-settings branch from 7bc5696 to a4c4e8a Compare February 19, 2025 21:21
@noCharger noCharger self-assigned this Feb 19, 2025
joshuali925
joshuali925 approved these changes Feb 19, 2025
View reviewed changes
docs/user/admin/settings.rst
@@ -16,36 +16,6 @@ Introduction

When OpenSearch bootstraps, SQL plugin will register a few settings in OpenSearch cluster settings. Most of the settings are able to change dynamically so you can control the behavior of SQL plugin without need to bounce your cluster. You can update the settings by sending requests to either ``_cluster/settings`` or ``_plugins/_query/settings`` endpoint, though the examples are sending to the latter.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to list the removed settings in breaking change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to list the removed settings in breaking change?

As long as we include this change in the release notes, I think it is less confusing to remove them from the documents.

integ-test/src/test/java/org/opensearch/sql/bwc/SQLBackwardsCompatibilityIT.java
@@ -93,21 +93,33 @@ public void testBackwardsCompatibility() throws Exception {
List<Map<String, Object>> plugins = (List<Map<String, Object>>) response.get("plugins");
Set<Object> pluginNames =
plugins.stream().map(map -> map.get("name")).collect(Collectors.toSet());
String version = (String) response.get("version");

boolean isBackwardsIncompatibleVersion = version.startsWith("2.");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would we test 2.x using main code? I thought we would remove 2.x from build.gradle so this won't happen, since they are not backward compatible

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would we test 2.x using main code? I thought we would remove 2.x from build.gradle so this won't happen, since they are not backward compatible

I think the majority of the features are still backward compatible, besides those planned to deparcate in 3.0. Maybe discuss the bwc version of main branch on thread #1880.

@noCharger noCharger merged commit c8e4e9a into opensearch-project:main Feb 20, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ykmr1224 ykmr1224 ykmr1224 approved these changes

@joshuali925 joshuali925 joshuali925 approved these changes

@penghuo penghuo penghuo left review comments

@ps48 ps48 Awaiting requested review from ps48 ps48 is a code owner

@kavithacm kavithacm Awaiting requested review from kavithacm kavithacm is a code owner

@derek-ho derek-ho Awaiting requested review from derek-ho derek-ho is a code owner

@dai-chen dai-chen Awaiting requested review from dai-chen dai-chen is a code owner

@YANG-DB YANG-DB Awaiting requested review from YANG-DB YANG-DB is a code owner

@mengweieric mengweieric Awaiting requested review from mengweieric mengweieric is a code owner

@Swiddis Swiddis Awaiting requested review from Swiddis Swiddis is a code owner

@seankao-az seankao-az Awaiting requested review from seankao-az seankao-az is a code owner

@MaxKsyunz MaxKsyunz Awaiting requested review from MaxKsyunz MaxKsyunz is a code owner

@Yury-Fridlyand Yury-Fridlyand Awaiting requested review from Yury-Fridlyand Yury-Fridlyand is a code owner

@anirudha anirudha Awaiting requested review from anirudha anirudha is a code owner

@forestmvey forestmvey Awaiting requested review from forestmvey forestmvey is a code owner

@acarbonetto acarbonetto Awaiting requested review from acarbonetto acarbonetto is a code owner

@GumpacG GumpacG Awaiting requested review from GumpacG GumpacG is a code owner

@LantaoJin LantaoJin Awaiting requested review from LantaoJin LantaoJin is a code owner

Assignees

@noCharger noCharger

Labels
v3.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@noCharger @penghuo @ykmr1224 @joshuali925