feat(java): implement Async Connection Pooling using FixedChannelPool#2606
feat(java): implement Async Connection Pooling using FixedChannelPool#2606rythm-sachdeva wants to merge 1 commit intoapache:masterfrom
Conversation
| connectionPoolSize.orElse(5), // Default to 5 if not provided | ||
| 1000, // maxPendingAcquires | ||
| connectionTimeout.map(Duration::toMillis).orElse(3000L) // map Duration to millis |
There was a problem hiding this comment.
Do we want to 1) make the params configurable? 2) or put them as static final constants?
Also seems we have the default provided by the builder already?
There was a problem hiding this comment.
@rythm-sachdeva please address this comment. The numeric values should be declared as static final. If any value has a default in the builder, it should be aligned in both places. Also, please remove the inline comments, the code is self-explanatory.
|
|
||
| /** | ||
| * Connects to the server asynchronously. | ||
| * BroadCast Command to each connection |
There was a problem hiding this comment.
very minor nit: Broadcasts to keep it consistent :)
| lastFuture.add(sendAsync(commandCode, payload.retainedDuplicate())); | ||
| } | ||
| // 2. Return the last Future (caller can treat this like a single request) | ||
| return lastFuture.get(lastFuture.size() - 1); |
There was a problem hiding this comment.
Not sure if I understand correctly, but how does the last future guarantee on the behavior of all futures?
There was a problem hiding this comment.
Does not guarantee, I assumed that if last request is fulfilled the previous requests have been successful. Though not a good approach. If there is any better approach you suggest I am happy to take feedback and implement it in an optimal way.
| // private final AtomicLong requestIdGenerator = new AtomicLong(0); | ||
| // private final ConcurrentHashMap<Long, CompletableFuture<ByteBuf>> pendingRequests = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
Do we want to remove them?
There was a problem hiding this comment.
If this code isn't being used, please remove it rather than leaving it commented out.
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. If you need a review, please ensure CI is green and the PR is rebased on the latest master. Don't hesitate to ping the maintainers - either @core on Discord or by mentioning them directly here on the PR. Thank you for your contribution! |
2cdb131 to
50e8a36
Compare
|
Hi @mmodzelewski Can you please review . |
Hey @rythm-sachdeva, I will, but I need some time. I might not get round to it until the beginning of next week. |
50e8a36 to
ca4583f
Compare
|
@rythm-sachdeva Please see why CI is failing and fix the errors. |
@rythm-sachdeva I've added a verification if all ByteBufs are properly released (not leaked) and it appears that the new code does not release all resources. See the logs, or run the tests locally, you'll see reports of the leaks in there. |
ca4583f to
17832af
Compare
fix(java-sdk):Added Missing PoolMetrics and linting issues fix(java):fixed connection error fix(java):Fixed Linting Issues fix(java):fixed builder issues fix(java):fixed ci issues fix(java): fixed memory leaks
17832af to
8a80d3d
Compare
Description
Implements connection pooling for
AsyncTcpConnectionusing Netty'sFixedChannelPoolto handle concurrent async operations efficiently.Changes
FixedChannelPoolfor concurrent request handlingPoolMetricsclass to track:TCPConnectionPoolConfigwith builder pattern for customizing:ConcurrentLinkedQueuebroadcastAsync()method to execute commands across all pooled connections (useful for authentication)AsyncIggyTcpClient.Builder.connectionPoolSize()Implementation Details
FixedChannelPoolwithChannelHealthChecker.ACTIVEIggyResponseHandlerwith a response queueTesting
testConnectionPoolMetrics()to verify metrics tracking