set read timeout on client

[rymurr]

This is to address the issue found in apache/iceberg#2328

---

This change is 

[codecov]

Codecov Report

Merging #1016 (58d7dd1) into main (ac123d3) will increase coverage by 0.07%.
The diff coverage is 70.00%.

@@             Coverage Diff              @@
##               main    #1016      +/-   ##
============================================
+ Coverage     70.53%   70.60%   +0.07%     
  Complexity      122      122              
============================================
  Files           240      241       +1     
  Lines          8955     9003      +48     
  Branches        846      847       +1     
============================================
+ Hits           6316     6357      +41     
- Misses         2202     2210       +8     
+ Partials        437      436       -1     

Flag	Coverage Δ	Complexity Δ
java	69.09% <70.00%> (+0.09%)	0.00 <0.00> (ø)
python	84.08% <ø> (ø)	0.00 <ø> (ø)

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

Impacted Files	Coverage Δ	Complexity Δ
...ie/client/http/HttpClientReadTimeoutException.java	25.00% <25.00%> (ø)	0.00 <0.00> (?)
...in/java/org/projectnessie/client/NessieClient.java	69.04% <42.85%> (-5.96%)	0.00 <0.00> (ø)
...java/org/projectnessie/client/http/HttpClient.java	75.00% <85.71%> (+1.66%)	0.00 <0.00> (ø)
...ava/org/projectnessie/client/NessieHttpClient.java	69.69% <100.00%> (+0.46%)	0.00 <0.00> (ø)
...ava/org/projectnessie/client/http/HttpRequest.java	82.66% <100.00%> (+1.78%)	0.00 <0.00> (ø)
...a/org/projectnessie/versioned/impl/InternalL1.java	78.78% <0.00%> (ø)	0.00% <0.00%> (ø%)
...g/projectnessie/versioned/impl/InternalBranch.java	74.89% <0.00%> (+0.41%)	0.00% <0.00%> (ø%)
...ojectnessie/versioned/impl/TieredVersionStore.java	89.80% <0.00%> (+1.37%)	0.00% <0.00%> (ø%)
...projectnessie/versioned/impl/HistoryRetriever.java	85.43% <0.00%> (+1.74%)	0.00% <0.00%> (ø%)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ac123d3...58d7dd1. Read the comment docs.

[snazy]

Reviewable status: 0 of 6 files reviewed, 6 unresolved discussions (waiting on @rymurr)

a discussion (no related file):
Being able to configure network timeouts is a good improvement!
Mind making the connect-timeout configurable as well?

---

---

clients/client/src/main/java/org/projectnessie/client/NessieClient.java, line 68 at r1 (raw file):

    private String password;
    private boolean tracing;
    private int readTimeout = 10;

I'd prefer milliseconds here, just because almost every HTTP client implementation uses milliseconds.

---

clients/client/src/main/java/org/projectnessie/client/NessieClient.java, line 68 at r1 (raw file):

    private String password;
    private boolean tracing;
    private int readTimeout = 10;

Just thinking: sun.net.NetworkClient uses the system property sun.net.client.defaultReadTimeout here, maybe we should use the same property as well, but just default to a > 0 default value. WDYT?

---

clients/client/src/main/java/org/projectnessie/client/NessieClient.java, line 177 at r1 (raw file):

     * @return {@code this}
     */
    public Builder withReadTimeout(int readTimeout) {

Rename the param to readTimeoutMillis

---

clients/client/src/main/java/org/projectnessie/client/http/HttpClient.java, line 101 at r1 (raw file):

    private ObjectMapper mapper;
    private SSLContext sslContext;
    private int readTimeout = 10;

I'd maybe give it a bit more by default - maybe 15 or 20 seconds.

---

clients/client/src/test/java/org/projectnessie/client/http/TestHttpClient.java, line 70 at r1 (raw file):

  @Test
  void testTimeout() {
    HttpHandler handler = h -> {

Looks like this test runs for quite a long time. Guess, this probably needs some Mockito magic.

[rymurr]

Reviewable status: 0 of 6 files reviewed, 6 unresolved discussions (waiting on @rymurr and @snazy)

---

clients/client/src/main/java/org/projectnessie/client/NessieClient.java, line 68 at r1 (raw file):

Previously, snazy (Robert Stupp) wrote…

I'd prefer milliseconds here, just because almost every HTTP client implementation uses milliseconds.

Done.

---

clients/client/src/main/java/org/projectnessie/client/NessieClient.java, line 68 at r1 (raw file):

Previously, snazy (Robert Stupp) wrote…

Just thinking: sun.net.NetworkClient uses the system property sun.net.client.defaultReadTimeout here, maybe we should use the same property as well, but just default to a > 0 default value. WDYT?

Done.

---

clients/client/src/main/java/org/projectnessie/client/NessieClient.java, line 177 at r1 (raw file):

Previously, snazy (Robert Stupp) wrote…

Rename the param to readTimeoutMillis

Done.

---

clients/client/src/main/java/org/projectnessie/client/http/HttpClient.java, line 101 at r1 (raw file):

Previously, snazy (Robert Stupp) wrote…

I'd maybe give it a bit more by default - maybe 15 or 20 seconds.

Done.

---

clients/client/src/test/java/org/projectnessie/client/http/TestHttpClient.java, line 70 at r1 (raw file):

Previously, snazy (Robert Stupp) wrote…

Looks like this test runs for quite a long time. Guess, this probably needs some Mockito magic.

Done.

[snazy]

Reviewable status: 0 of 6 files reviewed, 4 unresolved discussions (waiting on @rymurr)

---

clients/client/src/main/java/org/projectnessie/client/NessieClient.java, line 70 at r2 (raw file):

    private boolean tracing;
    private int readTimeoutMillis = Integer.parseInt(System.getProperty("sun.net.client.defaultReadTimeout", "25000"));
    private int connectionTimeoutMillis = Integer.parseInt(System.getProperty("sun.net.client.defaultConnectionTimeout", "25000"));

That's maybe quite long for a TCP connect-timeout. 3 or 5 seconds is probably good enough.

---

clients/client/src/main/java/org/projectnessie/client/http/HttpRequest.java, line 149 at r2 (raw file):

    } catch (SocketTimeoutException e) {
      throw new HttpClientReadTimeoutException(
          String.format("Cannot finish request. Timeout while waiting for response with a timeout of %ds", readTimeoutMillis / 1000), e);

The exception message is wrong (both connect + read throw a SocketTimeoutExcepiton), maybe just mention both timeouts (in millis).

---

clients/client/src/test/java/org/projectnessie/client/http/TestHttpClient.java, line 80 at r2 (raw file):

      try (TestServer server = new TestServer(handler)) {
        get(server.getAddress(), 15000, 1).get().readEntity(ExampleBean.class);
        Assertions.fail();

the Assertions.fail() is superfluous here ;)

[rymurr]

Reviewable status: 0 of 6 files reviewed, 4 unresolved discussions (waiting on @rymurr and @snazy)

---

clients/client/src/main/java/org/projectnessie/client/NessieClient.java, line 70 at r2 (raw file):

Previously, snazy (Robert Stupp) wrote…

That's maybe quite long for a TCP connect-timeout. 3 or 5 seconds is probably good enough.

Done.

---

clients/client/src/main/java/org/projectnessie/client/http/HttpRequest.java, line 149 at r2 (raw file):

Previously, snazy (Robert Stupp) wrote…

The exception message is wrong (both connect + read throw a SocketTimeoutExcepiton), maybe just mention both timeouts (in millis).

Where did you see that? I saw a different exception for connect. ConnectException or something.

---

clients/client/src/test/java/org/projectnessie/client/http/TestHttpClient.java, line 80 at r2 (raw file):

Previously, snazy (Robert Stupp) wrote…

the Assertions.fail() is superfluous here ;)

yup, removed.

[snazy]

Reviewable status: 0 of 6 files reviewed, 2 unresolved discussions (waiting on @rymurr)

---

clients/client/src/main/java/org/projectnessie/client/http/HttpRequest.java, line 149 at r2 (raw file):

Previously, rymurr (Ryan Murray) wrote…

Where did you see that? I saw a different exception for connect. ConnectException or something.

In the javadoc for java.net.Socket#connect(java.net.SocketAddress, int). But since you mention it, IIRC it's really ConnectException - interesting, that it's different from the javadoc.