wenbingshen commented on code in PR #3951:
URL: https://github.com/apache/bookkeeper/pull/3951#discussion_r1189330164
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieClientImpl.java:
##########
@@ -205,7 +228,7 @@ public PerChannelBookieClientPool lookupClient(BookieId
addr) {
}
PerChannelBookieClientPool newClientPool =
new DefaultPerChannelBookieClientPool(conf, this, addr,
numConnectionsPerBookie);
- PerChannelBookieClientPool oldClientPool =
channels.putIfAbsent(addr, newClientPool);
+ PerChannelBookieClientPool oldClientPool =
channels.asMap().putIfAbsent(addr, newClientPool);
Review Comment:
> how much cost adds "asMap" ? is performing operations or is it no-op ? if
it is no-op then we could cache the result of "asMap" into a field. if it
performs operations, then we are adding work on the hot paths (anytime we
read/write) and we should find a better way to expire idle connections, maybe
not using a "Cache" but with a specific background task that scans the map and
removes idle connection
Here I made a mistake, we can't use asMap, it won't update the latest access
time of the key.
it should be replaced with the following code:
```java
PerChannelBookieClientPool oldClientPool =
channels.getIfPresent(addr); <= here a1
if (null == oldClientPool) {
channels.put(addr, newClientPool); <== here a2
clientPool = newClientPool;
// initialize the pool only after we put the pool into
the map
clientPool.initialize();
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
