aratno commented on code in PR #1743:
URL:
https://github.com/apache/cassandra-java-driver/pull/1743#discussion_r1479025356
##########
core/src/main/java/com/datastax/oss/driver/internal/core/loadbalancing/DefaultLoadBalancingPolicy.java:
##########
@@ -276,38 +303,23 @@ protected boolean isBusy(@NonNull Node node, @NonNull
Session session) {
protected boolean isResponseRateInsufficient(@NonNull Node node, long now) {
// response rate is considered insufficient when less than 2 responses
were obtained in
// the past interval delimited by RESPONSE_COUNT_RESET_INTERVAL_NANOS.
- if (responseTimes.containsKey(node)) {
- AtomicLongArray array = responseTimes.get(node);
- if (array.length() == 2) {
- long threshold = now - RESPONSE_COUNT_RESET_INTERVAL_NANOS;
- long leastRecent = array.get(0);
- return leastRecent - threshold < 0;
- }
- }
- return true;
+ AtomicLongArray array = responseTimes.getIfPresent(node);
+ if (array != null && array.length() == 2) {
+ long threshold = now - RESPONSE_COUNT_RESET_INTERVAL_NANOS;
+ long leastRecent = array.get(0);
+ return leastRecent - threshold < 0;
+ } else return true;
Review Comment:
Style nit: Invert the condition and use an early-return if response rate is
insufficient, so you don't have `else return true`
##########
core/src/main/java/com/datastax/oss/driver/internal/core/metrics/AbstractMetricUpdater.java:
##########
@@ -173,9 +173,8 @@ protected Timeout newTimeout() {
.getTimer()
.newTimeout(
t -> {
- if (t.isExpired()) {
- clearMetrics();
- }
+ clearMetrics();
+ cancelMetricsExpirationTimeout();
Review Comment:
What's the reasoning for this change?
##########
core/src/main/java/com/datastax/oss/driver/internal/core/util/concurrent/ReplayingEventFilter.java:
##########
@@ -82,6 +82,7 @@ public void markReady() {
consumer.accept(event);
}
} finally {
+ recordedEvents.clear();
Review Comment:
What's the reasoning for this change?
##########
core/src/main/java/com/datastax/oss/driver/internal/core/loadbalancing/DefaultLoadBalancingPolicy.java:
##########
@@ -96,14 +99,38 @@ 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, AtomicLongArray> 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, AtomicLongArray> cacheLoader =
Review Comment:
Style nit: use a separate class for the cache value here, rather than using
AtomicLongArray as a generic container. Seems like it can be something like
`NodeResponseRateSample`, with methods like `boolean hasSufficientResponses`. I
see this was present in the previous implementation, so not a required change
for this PR, just something I noticed.
--
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]
