absurdfarce commented on code in PR #1743:
URL:
https://github.com/apache/cassandra-java-driver/pull/1743#discussion_r1637559826
##########
core/src/main/java/com/datastax/oss/driver/internal/core/loadbalancing/DefaultLoadBalancingPolicy.java:
##########
@@ -96,14 +100,39 @@ public class DefaultLoadBalancingPolicy extends
BasicLoadBalancingPolicy impleme
private static final int MAX_IN_FLIGHT_THRESHOLD = 10;
private static final long RESPONSE_COUNT_RESET_INTERVAL_NANOS =
MILLISECONDS.toNanos(200);
- protected final Map<Node, AtomicLongArray> responseTimes = new
ConcurrentHashMap<>();
+ protected final LoadingCache<Node, NodeResponseRateSample> responseTimes;
protected final Map<Node, Long> upTimes = new ConcurrentHashMap<>();
private final boolean avoidSlowReplicas;
public DefaultLoadBalancingPolicy(@NonNull DriverContext context, @NonNull
String profileName) {
super(context, profileName);
this.avoidSlowReplicas =
profile.getBoolean(DefaultDriverOption.LOAD_BALANCING_POLICY_SLOW_AVOIDANCE,
true);
+ CacheLoader<Node, NodeResponseRateSample> cacheLoader =
+ new CacheLoader<Node, NodeResponseRateSample>() {
+ @Override
+ public NodeResponseRateSample load(Node key) {
+ NodeResponseRateSample sample = responseTimes.getIfPresent(key);
+ if (sample == null) {
+ sample = new NodeResponseRateSample();
+ } else {
+ sample.update();
+ }
+ return sample;
+ }
+ };
Review Comment:
Honestly I've gone round and round about this one over the past few days. I
agree that my impl isn't right but a LoadingCache (or at least a LoadingCache
used in the way it's used here) still feels wrong to me. I _vastly_ prefer the
approach of the original code which just leverages ConcurrentHashMap.compute()
to do the right thing.
I think part of the mismatch for me here is that CacheLoader.get() doesn't
provide the current value (if any) as an arg so you wind up having to do this
weird refresh() dance. I'm assuming part of the reason the key wasn't provided
is because that's not really the intended use case for a CacheLoader but I
digress. As mentioned ConcurrentHashMap seems to model this case more
effectively, and while that class doesn't support weak references directly we
can get there indirectly with Guava'a MapMaker: `new
MapMaker().weakKeys.getMap()` will give us what we want. The only thing we're
missing then is the RemovalListener, but as mentioned elsewhere (a) I'm not
sure if that's necessary now and (b) I'm not sure it'll do what we want anyway.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
