Skip to content

Conversation

@brianf-aws
Copy link
Contributor

Description

This is a mitigation for #4170

But would need a more thorough code change so that clients can handle properly all situations.

Related Issues

#4170 can be closed but discussion can be done to consider different types of failures

Testing

./gradlew spotlessApply
./gradlew test

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.

@brianf-aws brianf-aws had a problem deploying to ml-commons-cicd-env-require-approval September 18, 2025 20:28 — with GitHub Actions Failure
@brianf-aws brianf-aws had a problem deploying to ml-commons-cicd-env-require-approval September 18, 2025 20:28 — with GitHub Actions Failure
@brianf-aws brianf-aws had a problem deploying to ml-commons-cicd-env-require-approval September 18, 2025 20:28 — with GitHub Actions Error
@brianf-aws brianf-aws had a problem deploying to ml-commons-cicd-env-require-approval September 18, 2025 20:28 — with GitHub Actions Error
ModelTensors tensors = processOutput(action, body, connector, scriptService, parameters, mlGuard);
tensors.setStatusCode(statusCode);
actionListener.onResponse(new Tuple<>(executionContext.getSequence(), tensors));
} catch (IllegalArgumentException e) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we also try to catch the OpensearchStatus Exception too in another catch block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah good catch would need to look what scenarios cause a OpensearchStatus to occur

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After checking again the try block does not do any other operations that would produce OpensearchStatusException. All the edge cases would only result in a illegalArgumentException

ModelTensors tensors = processOutput(action, body, connector, scriptService, parameters, mlGuard);

If we happen to find such error we will be able to see if through the log statement on the generic exception which would be very unlikely. The only other method that can throw a exception would be the

connector.parseResponse method which throws a IOException

connector.parseResponse(filteredResponse, modelTensors, scriptReturnModelTensor);

TLDR: adding a catch block OpensearchStatusException would be dead code, and making a test for it wouldnt be useful as we would just be mocking a scenario that doesn't exist

@dhrubo-os dhrubo-os had a problem deploying to ml-commons-cicd-env-require-approval September 23, 2025 20:18 — with GitHub Actions Failure
@dhrubo-os dhrubo-os had a problem deploying to ml-commons-cicd-env-require-approval September 23, 2025 20:18 — with GitHub Actions Failure
@dhrubo-os dhrubo-os had a problem deploying to ml-commons-cicd-env-require-approval September 23, 2025 20:18 — with GitHub Actions Failure
@dhrubo-os dhrubo-os had a problem deploying to ml-commons-cicd-env-require-approval September 23, 2025 20:18 — with GitHub Actions Failure
Signed-off-by: Brian Flores <[email protected]>
@brianf-aws brianf-aws had a problem deploying to ml-commons-cicd-env-require-approval September 24, 2025 19:14 — with GitHub Actions Error
@brianf-aws brianf-aws temporarily deployed to ml-commons-cicd-env-require-approval September 24, 2025 19:14 — with GitHub Actions Inactive
@brianf-aws brianf-aws temporarily deployed to ml-commons-cicd-env-require-approval September 24, 2025 19:14 — with GitHub Actions Inactive
@brianf-aws brianf-aws had a problem deploying to ml-commons-cicd-env-require-approval September 24, 2025 19:14 — with GitHub Actions Failure
@codecov
Copy link

codecov bot commented Sep 24, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.01%. Comparing base (247a88c) to head (7b98e78).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #4182   +/-   ##
=========================================
  Coverage     82.01%   82.01%           
- Complexity     9183     9195   +12     
=========================================
  Files           789      789           
  Lines         39567    39569    +2     
  Branches       4389     4389           
=========================================
+ Hits          32450    32454    +4     
+ Misses         5227     5225    -2     
  Partials       1890     1890           
Flag Coverage Δ
ml-commons 82.01% <100.00%> (+<0.01%) ⬆️

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.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@brianf-aws brianf-aws had a problem deploying to ml-commons-cicd-env-require-approval September 24, 2025 20:17 — with GitHub Actions Failure
@brianf-aws brianf-aws had a problem deploying to ml-commons-cicd-env-require-approval September 24, 2025 20:17 — with GitHub Actions Failure
@dhrubo-os dhrubo-os merged commit f83026c into opensearch-project:main Sep 24, 2025
9 of 13 checks passed
@opensearch-trigger-bot
Copy link
Contributor

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

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

@opensearch-trigger-bot
Copy link
Contributor

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

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

@opensearch-trigger-bot
Copy link
Contributor

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

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

opensearch-trigger-bot bot pushed a commit that referenced this pull request Sep 24, 2025
…4182)

* make MLSdkAsyncHttpResponseHandler return IllegalArgumentException

Signed-off-by: Brian Flores <[email protected]>

* empty commit to trigger CI

Signed-off-by: Brian Flores <[email protected]>

---------

Signed-off-by: Brian Flores <[email protected]>
Co-authored-by: Dhrubo Saha <[email protected]>
(cherry picked from commit f83026c)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants