feat(java): Support connection timeout

[jrhenderson1988]

Which issue does this PR close?

Closes #3010

Rationale

The connection timeout is a value supported by the builders of both the async and blocking TCP builders, but the clients that they build do not properly apply the connection timeout.

What changed?

Passes through the connection timeout into the AsyncIggyTcpClient and the subsequent AsyncTcpConnection where it is used in the bootstrap given to the connection pool. The ChannelOption.CONNECT_TIMEOUT_MILLIS value is set with the value of the connection timeout in milliseconds, falling back to the 3000ms value that was used previously if it's not set.

Local Execution

• Passed
• Pre-commit hooks ran

AI Usage

• Claude was used to ask some questions
• Jetbrains full line completion local model was used for autocomplete
• I can explain all code and full test suite was run to verify that everything still works

[jrhenderson1988]

@mmodzelewski I've added a few extra tests in this PR where I noticed there was some missing coverage.

On the flip side, I didn't quite know how we could actually test the connection timeout using the integration style tests that we run. Not sure if we have a current way of forcing a slow connection or introducing some latency to make the timeout kick in. I know that there are tools that could be used like Toxiproxy for example, but not sure if it would be desirable to add it (and I'm not that familiar with it). If you have any ideas, do let me know

[codecov]

Codecov Report

❌ Patch coverage is 86.66667% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.75%. Comparing base (b55d065) to head (c1b771b).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
...ache/iggy/client/async/tcp/AsyncTcpConnection.java	50.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##             master    #3026   +/-   ##
=========================================
  Coverage     71.74%   71.75%           
- Complexity      930      943   +13     
=========================================
  Files          1121     1121           
  Lines         93777    93800   +23     
  Branches      71125    71125           
=========================================
+ Hits          67284    67307   +23     
- Misses        23856    23858    +2     
+ Partials       2637     2635    -2     

Components	Coverage Δ
Rust Core	72.34% <ø> (ø)
Java SDK	62.30% <86.66%> (+0.21%)	⬆️
C# SDK	67.64% <ø> (ø)
Python SDK	81.43% <ø> (ø)
Node SDK	91.53% <ø> (ø)
Go SDK	38.68% <ø> (ø)

Files with missing lines	Coverage Δ
...ache/iggy/client/async/tcp/AsyncIggyTcpClient.java	98.55% <100.00%> (+0.02%)	⬆️
...gy/client/async/tcp/AsyncIggyTcpClientBuilder.java	91.54% <100.00%> (+13.77%)	⬆️
...ache/iggy/client/async/tcp/AsyncTcpConnection.java	71.02% <50.00%> (-2.18%)	⬇️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[jrhenderson1988]

Currently getting build failures on Cyclomatic and NPath complexity in the build method, since I added an additional validation for the connectionTimeout. If you want, I can refactor the method to call individual validate methods if we'd like to keep these metrics/gates?

[ex172000]

Overall LGTM, just some minor issues including several comments copy-pasting in tests

[mmodzelewski]

@ex172000 did you mean to add any comments to the code?

@jrhenderson1988 let's try to refactor the code to keep the existing gate levels.
Regarding the connection timeout, from our side it is mostly passing an argument, the heavy lifting is on the netty's side and I'd expect it is well tested. I wonder if netty has any test utils that would help us at least verify if we handle the timeout error correctly.

[jrhenderson1988]

Thank you both for the reviews.

@ex172000 I think I've fixed the comment copy/paste issue. I could only see one occurrence.

@mmodzelewski I've refactored the logic to keep the complexity of the method down below the gates. I had a look to see if there was something we could use from Netty to get it to simulate a connection failure but it doesn't look like there was anything. I did manage to surface a faillure by making it connect to 192.0.2.1 with a 100ms timeout and it threw an exception:

connection timed out after 100 ms: /192.0.2.1:8080
io.netty.channel.ConnectTimeoutException: connection timed out after 100 ms: /192.0.2.1:8080

Not sure if that's something we'd like to wrap and throw an Iggy based exception instead. What do you think?

Let me know if you would like any further changes. If not, this is ready to review again.

[ex172000]

@ex172000 did you mean to add any comments to the code?
Oh previously I mean the comments in tests does not match the implementation, thanks for the updates!

[mmodzelewski]

Not sure if that's something we'd like to wrap and throw an Iggy based exception instead. What do you think?

We've got IggyConnectionException in place, so I think it would be good to wrap the netty exception with it.

[jrhenderson1988]

@mmodzelewski I've just pushed an update where we check for Netty's ConnectTimeoutException when there's a failure to acquire a channel in the connect method and wrap it in IggyConnectionException. Hopefully that will do the job. I will say that I'm not super familiar with with Netty at this level so I could have gotten it wrong. Also unsure if/how that code path could be unit tested. The existing code within that else block is not being hit either.

[mmodzelewski]

@jrhenderson1988 Thanks, I think that looks good and we can merge. Additional tests can be introduced later.