Adding connector http timeout in the connector level #1835

dhrubo-os · 2024-01-04T00:50:21Z

Description

[Adding connector http timeout in the connector level]

Issues Resolved

[List any issues this PR will resolve]

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.

zane-neo · 2024-01-04T06:49:48Z

LGTM

codecov · 2024-01-04T19:41:04Z

Codecov Report

Attention: Patch coverage is 84.25197% with 20 lines in your changes are missing coverage. Please review.

Project coverage is 81.80%. Comparing base (0713dcd) to head (35d84e8).
Report is 2 commits behind head on main.

❗ Current head 35d84e8 differs from pull request most recent head 1dd9f34. Consider uploading reports for the commit 1dd9f34 to get more accurate results

Files	Patch %	Lines
...ml/common/connector/ConnectorHttpClientConfig.java	77.35%	8 Missing and 4 partials ⚠️
...engine/algorithms/remote/AwsConnectorExecutor.java	72.72%	6 Missing ⚠️
.../opensearch/ml/common/connector/HttpConnector.java	88.88%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1835      +/-   ##
============================================
- Coverage     81.87%   81.80%   -0.07%     
+ Complexity     5685     5640      -45     
============================================
  Files           545      545              
  Lines         22939    22913      -26     
  Branches       2364     2349      -15     
============================================
- Hits          18781    18745      -36     
- Misses         3216     3229      +13     
+ Partials        942      939       -3

Flag	Coverage Δ
ml-commons	`81.80% <84.25%> (-0.07%)`	⬇️

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.

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

Zhangxunmt

Once the ClientConfig is set in the connector, have you checked if this config can be updated using the UpdateConnector API? If not, we should add this field in the Update API to allow updates.

Zhangxunmt · 2024-03-01T21:24:01Z

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

+    private Integer readTimeout;
+
+    @Builder(toBuilder = true)
+    public ConnectorClientConfig(


Should we add a checkup here for the values? It seems all values are allowed to build this clientConfig. In some cases we should block CX to use a super large timeout value to avoid malicious requests?

Good point. I'll start a thread on this with our security engineer. If there's anything we need to address, we can do that in the following PR.

dhrubo-os · 2024-03-01T21:48:52Z

Once the ClientConfig is set in the connector, have you checked if this config can be updated using the UpdateConnector API? If not, we should add this field in the Update API to allow updates.

Yup, I tested withing providing the following body in the update request. It successfully updates the values.

{
  "client_config": {
    "max_connection": 1,
    "connection_timeout": 10000,
    "read_timeout": 10000
  }
  
}

b4sjoo · 2024-03-03T06:25:19Z

common/src/main/java/org/opensearch/ml/common/transport/connector/MLCreateConnectorInput.java

@@ -248,6 +266,13 @@ public void writeTo(StreamOutput output) throws IOException {
        }
        output.writeBoolean(dryRun);
        output.writeBoolean(updateConnector);
+        if (connectorClientConfig != null) {


Maybe add a version filter here to avoid bwc issue?

We have default values here. Could you please explain a bit more why BWC issue could happen here?

It's not because if there is value or not, it's because you are introduce new field here. If a master node with this version sends this input to a older data node, it will throw an exception due to cannot process

b4sjoo · 2024-03-03T06:25:55Z

common/src/main/java/org/opensearch/ml/common/transport/connector/MLCreateConnectorInput.java

@@ -277,5 +302,8 @@ public MLCreateConnectorInput(StreamInput input) throws IOException {
        }
        dryRun = input.readBoolean();
        updateConnector = input.readBoolean();
+        if (input.readBoolean()) {


Same here, you might need to consider add a version filter

b4sjoo · 2024-03-06T22:04:13Z

Approved with major concern: we need to add a version filter to check the input/output stream version in order to avoid bwc issue before this code deployed to prod. According to the code owner, in this case he will publish a separate PR for this version filter in 2.x branch, because in main branch 2.13 version is not ready due to some unknown issue.

opensearch-trigger-bot · 2024-03-06T22:07:37Z

The backport to 2.x 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.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-1835-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 25f21223367fb334d1276b2543adfa9357d1322e
# Push it to GitHub
git push --set-upstream origin backport/backport-1835-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-1835-to-2.x.

opensearch-trigger-bot · 2024-03-06T22:11:15Z

The backport to 2.x 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.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-1835-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 25f21223367fb334d1276b2543adfa9357d1322e
# Push it to GitHub
git push --set-upstream origin backport/backport-1835-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-1835-to-2.x.

opensearch-trigger-bot · 2024-03-06T23:41:05Z

The backport to 2.x 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.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-1835-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 25f21223367fb334d1276b2543adfa9357d1322e
# Push it to GitHub
git push --set-upstream origin backport/backport-1835-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-1835-to-2.x.

…ect#1835) * working on connector http timeout Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * adding more test + fixing integration test issue Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * updating default values Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * fixing unit tests Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * input format changed Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * removed unused code Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * fixing test Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * refactored more and add more tests Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * applying spotlessApply Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * addressing comments Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * removing spaces Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * addressed comments + updated open api model name and endpoints (deprecated) Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * updating test Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * adding fields Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * addresseing comments to rename client config Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> --------- Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> (cherry picked from commit 25f2122)

…ect#1835) * working on connector http timeout Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * adding more test + fixing integration test issue Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * updating default values Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * fixing unit tests Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * input format changed Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * removed unused code Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * fixing test Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * refactored more and add more tests Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * applying spotlessApply Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * addressing comments Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * removing spaces Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * addressed comments + updated open api model name and endpoints (deprecated) Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * updating test Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * adding fields Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * addresseing comments to rename client config Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> --------- Signed-off-by: Dhrubo Saha <dhrubo@amazon.com>

…1835) (#2183) * Adding connector http timeout in the connector level (#1835) * working on connector http timeout Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * adding more test + fixing integration test issue Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * updating default values Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * fixing unit tests Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * input format changed Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * removed unused code Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * fixing test Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * refactored more and add more tests Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * applying spotlessApply Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * addressing comments Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * removing spaces Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * addressed comments + updated open api model name and endpoints (deprecated) Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * updating test Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * adding fields Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * addresseing comments to rename client config Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> --------- Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * fix compilation issue Signed-off-by: zane-neo <zaniu@amazon.com> --------- Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> Signed-off-by: zane-neo <zaniu@amazon.com> Co-authored-by: Dhrubo Saha <dhrubo@amazon.com>

…ect#1835) * working on connector http timeout Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * adding more test + fixing integration test issue Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * updating default values Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * fixing unit tests Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * input format changed Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * removed unused code Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * fixing test Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * refactored more and add more tests Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * applying spotlessApply Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * addressing comments Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * removing spaces Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * addressed comments + updated open api model name and endpoints (deprecated) Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * updating test Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * adding fields Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * addresseing comments to rename client config Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> --------- Signed-off-by: Dhrubo Saha <dhrubo@amazon.com>

ylwu-amzn · 2024-03-19T17:07:46Z

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

@@ -156,6 +156,9 @@ public class CommonValue {
                        + AbstractConnector.CREDENTIAL_FIELD
                        + "\" : {\"type\": \"flat_object\"},\n"
                        + "      \""
+                        + AbstractConnector.CLIENT_CONFIG_FIELD


Should bump ML_CONNECTOR_SCHEMA_VERSION

Fixed in PR #2228

dhrubo-os had a problem deploying to ml-commons-cicd-env January 4, 2024 00:50 — with GitHub Actions Error

dhrubo-os had a problem deploying to ml-commons-cicd-env January 4, 2024 00:50 — with GitHub Actions Failure

dhrubo-os changed the title ~~Adding connector http timeout in the connector level~~ [Draft] Adding connector http timeout in the connector level Jan 4, 2024

dhrubo-os had a problem deploying to ml-commons-cicd-env January 4, 2024 00:50 — with GitHub Actions Error

dhrubo-os had a problem deploying to ml-commons-cicd-env January 4, 2024 00:50 — with GitHub Actions Failure

austintlee mentioned this pull request Jan 4, 2024

[FEATURE] Support for timeout in http connector #1470

Closed

dhrubo-os had a problem deploying to ml-commons-cicd-env January 4, 2024 08:39 — with GitHub Actions Error

dhrubo-os had a problem deploying to ml-commons-cicd-env January 4, 2024 08:39 — with GitHub Actions Failure

dhrubo-os had a problem deploying to ml-commons-cicd-env January 4, 2024 08:39 — with GitHub Actions Error

dhrubo-os had a problem deploying to ml-commons-cicd-env January 4, 2024 08:39 — with GitHub Actions Failure

dhrubo-os had a problem deploying to ml-commons-cicd-env January 4, 2024 08:39 — with GitHub Actions Error

dhrubo-os had a problem deploying to ml-commons-cicd-env January 4, 2024 17:51 — with GitHub Actions Error

dhrubo-os had a problem deploying to ml-commons-cicd-env January 4, 2024 17:51 — with GitHub Actions Failure

dhrubo-os had a problem deploying to ml-commons-cicd-env January 4, 2024 17:51 — with GitHub Actions Error

dhrubo-os had a problem deploying to ml-commons-cicd-env January 4, 2024 17:52 — with GitHub Actions Error

dhrubo-os had a problem deploying to ml-commons-cicd-env January 4, 2024 17:52 — with GitHub Actions Failure

dhrubo-os had a problem deploying to ml-commons-cicd-env January 4, 2024 19:20 — with GitHub Actions Error

dhrubo-os had a problem deploying to ml-commons-cicd-env January 4, 2024 19:20 — with GitHub Actions Failure

dhrubo-os had a problem deploying to ml-commons-cicd-env January 4, 2024 19:20 — with GitHub Actions Error

dhrubo-os temporarily deployed to ml-commons-cicd-env January 4, 2024 19:20 — with GitHub Actions Inactive

dhrubo-os had a problem deploying to ml-commons-cicd-env January 4, 2024 19:42 — with GitHub Actions Failure

addresseing comments to rename client config

1dd9f34

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

dhrubo-os force-pushed the main branch from 35d84e8 to 1dd9f34 Compare March 1, 2024 20:48

dhrubo-os temporarily deployed to ml-commons-cicd-env March 1, 2024 20:48 — with GitHub Actions Inactive

dhrubo-os had a problem deploying to ml-commons-cicd-env March 1, 2024 20:48 — with GitHub Actions Error

dhrubo-os had a problem deploying to ml-commons-cicd-env March 1, 2024 20:48 — with GitHub Actions Failure

dhrubo-os had a problem deploying to ml-commons-cicd-env March 1, 2024 20:48 — with GitHub Actions Error

Zhangxunmt reviewed Mar 1, 2024

View reviewed changes

Zhangxunmt approved these changes Mar 1, 2024

View reviewed changes

b4sjoo reviewed Mar 3, 2024

View reviewed changes

b4sjoo approved these changes Mar 6, 2024

View reviewed changes

dhrubo-os merged commit 25f2122 into opensearch-project:main Mar 6, 2024
7 of 10 checks passed

dhrubo-os added the backport 2.x label Mar 6, 2024

dhrubo-os added backport 2.x and removed backport 2.x labels Mar 6, 2024

ylwu-amzn reviewed Mar 19, 2024

View reviewed changes

dhrubo-os mentioned this pull request Mar 20, 2024

2.13 release note #2231

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding connector http timeout in the connector level #1835

Adding connector http timeout in the connector level #1835

dhrubo-os commented Jan 4, 2024 •

edited

Loading

zane-neo commented Jan 4, 2024

codecov bot commented Jan 4, 2024 •

edited

Loading

Zhangxunmt left a comment

Zhangxunmt Mar 1, 2024

dhrubo-os Mar 1, 2024

dhrubo-os commented Mar 1, 2024

b4sjoo Mar 3, 2024

dhrubo-os Mar 3, 2024

b4sjoo Mar 4, 2024

b4sjoo Mar 3, 2024

b4sjoo commented Mar 6, 2024

opensearch-trigger-bot bot commented Mar 6, 2024

opensearch-trigger-bot bot commented Mar 6, 2024

opensearch-trigger-bot bot commented Mar 6, 2024

ylwu-amzn Mar 19, 2024

ylwu-amzn Mar 19, 2024

Adding connector http timeout in the connector level #1835

Adding connector http timeout in the connector level #1835

Conversation

dhrubo-os commented Jan 4, 2024 • edited Loading

Description

Issues Resolved

Check List

zane-neo commented Jan 4, 2024

codecov bot commented Jan 4, 2024 • edited Loading

Codecov Report

Zhangxunmt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhrubo-os commented Mar 1, 2024

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

b4sjoo commented Mar 6, 2024

opensearch-trigger-bot bot commented Mar 6, 2024

opensearch-trigger-bot bot commented Mar 6, 2024

opensearch-trigger-bot bot commented Mar 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhrubo-os commented Jan 4, 2024 •

edited

Loading

codecov bot commented Jan 4, 2024 •

edited

Loading