Skip to content

Conversation

zane-neo
Copy link
Collaborator

Description

Fix connector regex bypass issue

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: zane-neo <[email protected]>
@zane-neo zane-neo temporarily deployed to ml-commons-cicd-env October 22, 2025 04:56 — with GitHub Actions Inactive
@zane-neo zane-neo had a problem deploying to ml-commons-cicd-env October 22, 2025 04:56 — with GitHub Actions Failure
@zane-neo zane-neo temporarily deployed to ml-commons-cicd-env October 22, 2025 04:56 — with GitHub Actions Inactive
@zane-neo zane-neo temporarily deployed to ml-commons-cicd-env October 22, 2025 06:50 — with GitHub Actions Inactive
@zane-neo zane-neo temporarily deployed to ml-commons-cicd-env October 22, 2025 06:50 — with GitHub Actions Inactive
@codecov
Copy link

codecov bot commented Oct 22, 2025

Codecov Report

❌ Patch coverage is 77.63158% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.12%. Comparing base (66bcde7) to head (6289562).

Files with missing lines Patch % Lines
...rch/ml/common/httpclient/ValidatingHttpClient.java 65.11% 11 Missing and 4 partials ⚠️
...engine/algorithms/remote/AwsConnectorExecutor.java 91.66% 1 Missing ⚠️
...e/algorithms/remote/HttpJsonConnectorExecutor.java 90.90% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #4336      +/-   ##
============================================
- Coverage     80.14%   80.12%   -0.03%     
- Complexity    10172    10174       +2     
============================================
  Files           854      855       +1     
  Lines         44219    44239      +20     
  Branches       5113     5113              
============================================
+ Hits          35440    35447       +7     
- Misses         6630     6641      +11     
- Partials       2149     2151       +2     
Flag Coverage Δ
ml-commons 80.12% <77.63%> (-0.03%) ⬇️

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.

Duration connectionTimeout,
Duration readTimeout,
int maxConnections,
AtomicBoolean connectorPrivateIpEnabled
Copy link
Contributor

Choose a reason for hiding this comment

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

can we use bool here? what's the reason to use atomicBoolean

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agree that boolean is better, I tried to change all the occurrence of this connectorPrivateIpEnabled to boolean all the way back to where it's defined, but there's more UTs need to be changed. But changing to boolean is correct direction, let me try again this change.

import software.amazon.awssdk.http.async.SdkAsyncHttpClient;

@Log4j2
public class ValidatingHttpClient implements SdkAsyncHttpClient {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggest use a better class name.

Copy link
Contributor

@rithin-pullela-aws rithin-pullela-aws left a comment

Choose a reason for hiding this comment

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

Other than the comment from @ylwu-amzn, looks good to me!

@ylwu-amzn
Copy link
Collaborator

let's backport to 3.3

@zane-neo zane-neo had a problem deploying to ml-commons-cicd-env October 23, 2025 03:52 — with GitHub Actions Failure
@zane-neo zane-neo temporarily deployed to ml-commons-cicd-env October 23, 2025 03:52 — with GitHub Actions Inactive
@zane-neo zane-neo deployed to ml-commons-cicd-env October 23, 2025 03:52 — with GitHub Actions Active
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