Enable jib.httpTimeout system property for HTTP connection and read timeout

[chanseokoh]

Part of #582.

Decided to read the system property in RegistryAuthenticator and RegistryEndpointCaller.

[chanseokoh]

Hold on. Chatted with @coollog, and I'll rework this.

[chanseokoh]

I think this is good for review.

The next step after this PR is to check the system property value at the frontend to print out warnings, if necessary.

[coollog]

Is there a way we could reuse the default from the Google HTTP client? Or not set an HTTP timeout to allow it to use the default from the HTTP library?

[chanseokoh]

The Google HTTP client doesn't define the public static constant, so I'll pursue the latter approach.

[coollog]

Looks good! Just a small question.

[coollog]

Is using null a common way to do this? If not, I was thinking maybe use negative to indicate absence?

[chanseokoh]

I think it's not uncommon to use null to indicate a field is unset or uninitialized, or something is absent. Using a negative value works too. However, it won't be distinguishable from the user wrongly giving a negative value. That will matter if we error out on a negative timeout value as you said.

[coollog]

Okay sounds good 👍 let's add a javadoc explicitly mentioning that null means the caller should use some default.

[chanseokoh]

Ah, you weren't talking about a field here. I see what you meant. I'll think about the API-aspect of this method.

[coollog]

I think we could do just return Integer.getInteger("jib.httpTimeout", null) since in the negative case, we probably want to error (thrown by HttpRequest#setConnectTimeout)

[chanseokoh]

I was thinking showing a warning with a negative value, instead of errorring out. Do you think it's not good?

Anyways, if we decide to error out, I'll probably detect it and fail at the frontends with better error messages, rather than just displaying up an unfriendly stack trace from Preconditions.checkArgument() with no message.

[coollog]

I think either way is good. I lean towards the errorring since the case I could think of where negative could show up is if the user may have meant to type another timeout and accidentally put a minus sign in front - in which case, a warning may be lost in the logs to a different error when the connection fails.

[chanseokoh]

I see. I think then the same argument applies to non-parsable string values. The user could accidentally put any non-digit characters, so it also makes sense to error out on non-parsable strings. Going for errorring on both cases?

[coollog]

Yea, I think in the non-parsable version, this already errors, since Integer#getInteger would throw a NumberFormatException.

[chanseokoh]

Alright, errorring on both cases. (BTW, Integer.getInteger() doesn't throw NumberFormatException. parseInt() does, OTOH.)

[coollog]

Ah right, looks like the getInteger(String nm, Integer val) one catches and ignores the NumberFormatException

[chanseokoh]

Will update the code tomorrow, but I'm thinking of doing

      Integer httpTimeout = null; // implies using the default timeout
      String timeoutValue = System.getProperty("jib.httpTimeout");
      if (timeoutValue != null) {
        httpTimeout = Integer.parseInt(timeoutValue);
        if (httpTimeout < 0) {
          throw new RegistryAuthenticationFailedException("Negative value of jib.httpTimeout");
        }
      }
      ...
      try (Connection connection = ...) {
        Request.Builder requestBuilder = Request.builder().setHttpTimeout(httpTimeout);
      ...
    } catch (NumberFormatException ex) {
      throw new RegistryAuthenticationFailedException("Non-integer value of jib.httpTimeout");
    }

Since all versions of getInteger() don't throw NumberFormatException, I guess I need to call parseInt() after System.getProperty().

[chanseokoh]

@coollog and I agreed that we make the code simple and not care about the edge cases too much here. If something goes awry, so be it. Instead, we will guard the errors at the plugin frontends.

[coollog]

Oh let's add a CHANGELOG entry for this.