removed empty host check, inherently within httpHost object generation

[Arnav-Gr0ver]

Description

if statement to check if httpHost name is not needed as an exception is already raised when creating the httpHost object for null, empty, and whitespace hostNames

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.

[codecov]

Codecov Report

Merging #1488 (52a542a) into main (6e0d949) will increase coverage by 0.09%.
Report is 24 commits behind head on main.
The diff coverage is n/a.

❗ Current head 52a542a differs from pull request most recent head cb70083. Consider uploading reports for the commit cb70083 to get more accurate results

@@             Coverage Diff              @@
##               main    #1488      +/-   ##
============================================
+ Coverage     77.44%   77.54%   +0.09%     
- Complexity     2178     2180       +2     
============================================
  Files           173      173              
  Lines          8984     8981       -3     
  Branches        892      892              
============================================
+ Hits           6958     6964       +6     
+ Misses         1620     1614       -6     
+ Partials        406      403       -3     

Flag	Coverage Δ
ml-commons	77.54% <ø> (+0.09%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...arch/ml/engine/httpclient/MLHttpClientFactory.java	66.66% <ø> (+3.50%)	⬆️

... and 2 files with indirect coverage changes

[HenryL27]

why do we need to remove this check?
where do the exceptions get thrown from other than here?
will this make it easier or harder to debug things?

[Arnav-Gr0ver]

why do we need to remove this check? where do the exceptions get thrown from other than here? will this make it easier or harder to debug things?

Hello Henry!

Apologies for the late reply, in regards to your questions:

1. the check, i.e. the if statement with isBlank is redundant in regards to the checks already within the HttpHost class for the hostname (there is a method containsNoBlanks which already throws the exceptions we wanted to test for)

public HttpHost(final String hostname, final int port, final String scheme) {
        super();
        this.hostname   = Args.containsNoBlanks(hostname, "Host name");
        this.lcHostname = hostname.toLowerCase(Locale.ROOT);
        if (scheme != null) {
            this.schemeName = scheme.toLowerCase(Locale.ROOT);
        } else {
            this.schemeName = DEFAULT_SCHEME_NAME;
        }
        this.port = port;
        this.address = null;
    }

and the containsNoBlanks method

public static <T extends CharSequence> T containsNoBlanks(final T argument, final String name) {
        if (argument == null) {
            throw new IllegalArgumentException(name + " may not be null");
        }
        if (argument.length() == 0) {
            throw new IllegalArgumentException(name + " may not be empty");
        }
        if (TextUtils.containsBlanks(argument)) {
            throw new IllegalArgumentException(name + " may not contain blanks");
        }
        return argument;
    }

3. I don't have enough experience with the codebase to definitively say xyz would be easier, but I was running into some issues when writing unit tests for this code block - the exception was not being thrown and the lines weren't executing hence impacting coverage - and I thought that there was already enough specificity about the type of invalid hostName in containsNoBlanks over the general "host name is empty".

Would appreciate your feedback on this, and I went over this issue with @dhrubo-os & @jngz-es if they'd like to weigh in!

[jngz-es]

Thanks for this pr, it looks good to me. Just minor comment, you can put the issue link under the Issues Resolved section.

[ylwu-amzn]

Thanks @Arnav-Gr0ver , I think we should ask @zane-neo to help review too as he wrote this part. Zan, can you help review if it's ok to remove this check?

[dhrubo-os]

@Arnav-Gr0ver DCO is missing.

[ylwu-amzn]

@Arnav-Gr0ver DCO is missing.

@Arnav-Gr0ver Thanks for making this change, you can refer to this https://github.com/opensearch-project/ml-commons/pull/1488/checks?check_run_id=17613303436 about how to fix DCO

[austintlee]

LGTM.