yifan-c commented on code in PR #81:
URL: https://github.com/apache/cassandra-sidecar/pull/81#discussion_r1460009496
##########
client/src/main/java/org/apache/cassandra/sidecar/client/SidecarClient.java:
##########
@@ -80,9 +83,9 @@ public SidecarClient(SidecarInstancesProvider
instancesProvider,
public CompletableFuture<HealthResponse> sidecarHealth()
{
return executor.executeRequestAsync(requestBuilder()
- .sidecarHealthRequest()
- .retryPolicy(new
OncePerInstanceRetryPolicy())
- .build());
+ .sidecarHealthRequest()
+ .retryPolicy(oncePerInstanceRetryPolicy)
+ .build());
Review Comment:
The indentation looks wrong.
##########
common/src/main/java/org/apache/cassandra/sidecar/common/utils/TimeUtils.java:
##########
@@ -0,0 +1,36 @@
+package org.apache.cassandra.sidecar.common.utils;
+
+import java.time.Duration;
+import java.util.concurrent.ThreadLocalRandom;
+
+/**
+ * Utility class for manipulating dates, times, and durations
+ */
+public final class TimeUtils {
+ /**
+ * Private constructor that prevents unnecessary instantiation
+ *
+ * @throws IllegalStateException when called
+ */
+ private TimeUtils()
+ {
+ throw new IllegalStateException(getClass() + " is a static utility
class and shall not be instantiated");
+ }
+
+ /**
+ * Returns a random duration with millisecond precision that is uniformly
distributed between two provided durations
+ *
+ * @param minimum minimum possible duration, inclusive
+ * @param maximum maximum possible duration, inclusive
+ *
+ * @return random duration uniformly distributed between two provided
durations, with millisecond precision
+ *
+ * @throws IllegalArgumentException if minimum duration is greater than
maximum duration
+ */
+ public static Duration randomDuration(Duration minimum, Duration maximum)
+ {
+ Preconditions.checkArgument(minimum.compareTo(maximum) <= 0,
+ "Minimum duration must be less or equal to
maximum duration");
+ return
Duration.ofMillis(ThreadLocalRandom.current().nextLong(minimum.toMillis(),
maximum.toMillis() + 1L));
Review Comment:
I would like to suggest an alternative.
Instead of having `min` and `max` as the parameters of the method (and the
implementation has to check that `min` is indeed not greater than `max`), you
can pass `base` and `range`.
In addition to the implementation change, I do not see the necessity of
using thread local random. A shared `Random` object should be suffice. You do
not need to create a random object per calling thread.
```suggestion
public static Duration randomDuration(long base, long range)
{
long randomDurationMillis = base + random.nextLong() % range;
return Duration.ofMillis(randomDurationMillis);
```
##########
client/src/main/java/org/apache/cassandra/sidecar/client/SidecarClientConfigImpl.java:
##########
@@ -93,40 +121,62 @@ public Builder self()
}
/**
- * Sets the {@code maxRetries} and returns a reference to this Builder
enabling method chaining.
+ * Sets the {@code maxRetries} and returns a reference to this Builder
enabling method chaining
*
* @param maxRetries the {@code maxRetries} to set
* @return a reference to this Builder
*/
public Builder maxRetries(int maxRetries)
{
- return update(b -> b.maxRetries = maxRetries);
+ return update(builder -> builder.maxRetries = maxRetries);
Review Comment:
unnecessary (and unrelated) change
##########
client/src/main/java/org/apache/cassandra/sidecar/client/SidecarClientConfigImpl.java:
##########
@@ -68,19 +76,39 @@ public long maxRetryDelayMillis()
return maxRetryDelayMillis;
}
+ /**
+ * @return the minimum amount of time to wait before retrying a failed
health check
+ */
+ @Override
+ public Duration minimumHealthRetryDelay()
+ {
+ return minimumHealthRetryDelay;
+ }
+
+ /**
+ * @return the maximum amount of time to wait before retrying a failed
health check
+ */
+ @Override
+ public Duration maximumHealthRetryDelay()
+ {
+ return maximumHealthRetryDelay;
+ }
+
public static Builder builder()
{
return new Builder();
}
/**
- * {@code SidecarConfig} builder static inner class.
+ * {@code SidecarConfig} builder static inner class
Review Comment:
removing the period is unnecessary.
##########
client/src/main/java/org/apache/cassandra/sidecar/client/SidecarClient.java:
##########
@@ -63,13 +64,15 @@ public SidecarClient(SidecarInstancesProvider
instancesProvider,
RetryPolicy defaultRetryPolicy)
{
this.defaultRetryPolicy = defaultRetryPolicy;
- ignoreConflictRetryPolicy = new
IgnoreConflictRetryPolicy(sidecarClientConfig.maxRetries(),
-
sidecarClientConfig.retryDelayMillis(),
-
sidecarClientConfig.maxRetryDelayMillis());
- baseBuilder = new RequestContext.Builder()
- .instanceSelectionPolicy(new
RandomInstanceSelectionPolicy(instancesProvider))
- .retryPolicy(defaultRetryPolicy);
- executor = requestExecutor;
+ this.ignoreConflictRetryPolicy = new
IgnoreConflictRetryPolicy(sidecarClientConfig.maxRetries(),
+
sidecarClientConfig.retryDelayMillis(),
+
sidecarClientConfig.maxRetryDelayMillis());
+ this.oncePerInstanceRetryPolicy = new
OncePerInstanceRetryPolicy(sidecarClientConfig.minimumHealthRetryDelay(),
+
sidecarClientConfig.maximumHealthRetryDelay());
+ this.baseBuilder = new RequestContext.Builder()
+ .instanceSelectionPolicy(new
RandomInstanceSelectionPolicy(instancesProvider))
+ .retryPolicy(defaultRetryPolicy);
Review Comment:
The indentation looks wrong.
I also feel that adding the `this.` to instance variable unnecessarily
increases the number of line of changes.
--
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]
