frankgh commented on code in PR #61:
URL: https://github.com/apache/cassandra-sidecar/pull/61#discussion_r1262865162
##########
client/src/main/java/org/apache/cassandra/sidecar/client/HttpResponseImpl.java:
##########
@@ -32,17 +32,22 @@ public class HttpResponseImpl implements HttpResponse
private final String statusMessage;
private final byte[] raw;
private final Map<String, List<String>> headers;
+ private final SidecarInstance respondingServer;
Review Comment:
NIT: instance for consistency with the `SidecarInstance` class name
```suggestion
private final SidecarInstance sidecarInstance;
```
##########
client/src/main/java/org/apache/cassandra/sidecar/client/RequestExecutor.java:
##########
@@ -101,10 +100,20 @@ public <T> T executeRequest(RequestContext context, long
timeout, TimeUnit unit)
*/
public <T> CompletableFuture<T> executeRequestAsync(RequestContext context)
{
+ Iterator<SidecarInstance> iterator =
context.instanceSelectionPolicy().iterator();
+ SidecarInstance instance = iterator.next();
Review Comment:
This seems wrong. `iterator.next` if implemented according to spec, will
throw a `NoSuchElementException` if the iteration has no more elements. We must
call `hasNext` before calling `next`
##########
client/src/main/java/org/apache/cassandra/sidecar/client/RequestExecutor.java:
##########
@@ -122,9 +131,18 @@ public void streamRequest(RequestContext context,
StreamConsumer streamConsumer)
{
Objects.requireNonNull(streamConsumer, "streamConsumer must be
non-null");
Iterator<SidecarInstance> iterator =
context.instanceSelectionPolicy().iterator();
-
+ SidecarInstance instance = iterator.next();
Review Comment:
Again , we shouldn't call `next` without checking `hasNext`
##########
client/src/main/java/org/apache/cassandra/sidecar/client/HttpResponse.java:
##########
@@ -62,4 +62,9 @@ default String contentAsString()
* @return the headers for the response
*/
Map<String, List<String>> headers();
+
+ /**
+ * @return the server that returns the response
+ */
+ SidecarInstance respondingServer();
Review Comment:
Would it make more sense to call this instance instead?
```suggestion
SidecarInstance sidecarInstance();
```
##########
client/src/main/java/org/apache/cassandra/sidecar/client/exception/RetriesExhaustedException.java:
##########
@@ -28,24 +29,52 @@ public class RetriesExhaustedException extends
RuntimeException
/**
* Constructs an exception with the number of {@code attempts} performed
for the request.
*
- * @param attempts the number of attempts performed for the request
- * @param request the HTTP request
+ * @param attempts the number of attempts performed for the request
+ * @param request the HTTP request
+ * @param lastResponse the last failed HTTP response
*/
- public RetriesExhaustedException(int attempts, Request request)
+ public static RetriesExhaustedException of(int attempts,
+ Request request,
+ HttpResponse lastResponse)
{
- this(attempts, request, null);
+ return of(attempts, request, lastResponse, null);
Review Comment:
call the ctor directly?
##########
client/src/main/java/org/apache/cassandra/sidecar/client/RequestExecutor.java:
##########
@@ -142,62 +160,6 @@ public void close() throws Exception
httpClient.close();
}
- /**
- * Executes the request from the {@code context}, it iterates over the
{@link SidecarInstance}s until the response
- * satisfies the {@code retryPolicy}.
- *
- * @param future a future for the {@link HttpResponse}
- * @param iterator the iterator of instances
- * @param context the request context
- * @param attempt the number of attempts for this request
- * @param throwable the last {@link Throwable}, or {@code null} if there
are no previous errors
- */
- protected void executeWithRetries(CompletableFuture<HttpResponse> future,
- Iterator<SidecarInstance> iterator,
- RequestContext context,
- int attempt,
- Throwable throwable)
- {
- if (iterator.hasNext())
- {
- executeWithRetries(future, iterator, iterator.next(), context,
attempt);
- }
- else
- {
- // exhausted retries on all available hosts
- future.completeExceptionally(new
RetriesExhaustedException(attempt, context.request(), throwable));
- }
- }
Review Comment:
👍
##########
client/src/main/java/org/apache/cassandra/sidecar/client/RequestExecutor.java:
##########
@@ -305,26 +267,21 @@ private void
applyRetryPolicy(CompletableFuture<HttpResponse> future,
context.retryPolicy()
.onResponse(future, request, response, throwable, attempt,
retryOnNewHost, (nextAttempt, delay) -> {
String statusCode = response != null ?
String.valueOf(response.statusCode()) : "<Not Available>";
+ SidecarInstance nextInstance = sidecarInstance;
if (iterator.hasNext())
{
- if (response == null || response.statusCode() !=
HttpResponseStatus.ACCEPTED.code())
- {
- logger.warn("Retrying request on next instance after {}ms.
Failed on instance={}, " +
- "attempt={}, statusCode={}, request={}",
delay, sidecarInstance, attempt, statusCode,
- request, throwable);
- }
- schedule(delay, () -> executeWithRetries(future, iterator,
context, nextAttempt, throwable));
+ nextInstance = iterator.next();
}
- else
+
+ if (response == null || response.statusCode() !=
HttpResponseStatus.ACCEPTED.code())
{
- if (response == null || response.statusCode() !=
HttpResponseStatus.ACCEPTED.code())
- {
- logger.warn("Retrying request on same instance after {}ms.
Failed on instance={}, " +
- "attempt={}, statusCode={}, request={}",
delay, sidecarInstance, attempt, statusCode,
- request, throwable);
- }
- schedule(delay, () -> executeWithRetries(future, iterator,
sidecarInstance, context, nextAttempt));
+ logger.warn("Retrying request on {} instance after {}ms. " +
+ "Failed on instance={}, attempt={}, statusCode={}",
+ nextInstance == sidecarInstance ? "same" : "next",
delay,
+ nextInstance, attempt, statusCode, throwable);
Review Comment:
Wouldn't the failed instance be `sidecarInstance` instead?
```suggestion
sidecarInstance, attempt, statusCode, throwable);
```
##########
client/src/main/java/org/apache/cassandra/sidecar/client/HttpResponseImpl.java:
##########
@@ -32,17 +32,22 @@ public class HttpResponseImpl implements HttpResponse
private final String statusMessage;
private final byte[] raw;
private final Map<String, List<String>> headers;
+ private final SidecarInstance respondingServer;
/**
* Constructs a response object with the provided values
*
* @param statusCode the status code of the response
* @param statusMessage the status message of the response
* @param headers the headers from the response
+ * @param server the server that returns the response
Review Comment:
this javadoc seems to reference an inexistent param, and `./gradlew
javadocs` will probably fail
```suggestion
* @param server the server that returns the response
```
##########
client/src/main/java/org/apache/cassandra/sidecar/client/RequestExecutor.java:
##########
@@ -101,10 +100,20 @@ public <T> T executeRequest(RequestContext context, long
timeout, TimeUnit unit)
*/
public <T> CompletableFuture<T> executeRequestAsync(RequestContext context)
{
+ Iterator<SidecarInstance> iterator =
context.instanceSelectionPolicy().iterator();
+ SidecarInstance instance = iterator.next();
CompletableFuture<T> resultFuture = new CompletableFuture<>();
+ if (instance == null)
+ {
+ resultFuture.completeExceptionally(new
IllegalStateException("InstanceSelectionPolicy " +
+
context.instanceSelectionPolicy()
+
.getClass()
+
.getSimpleName() +
+ "
selects 0 instance"));
Review Comment:
```suggestion
"
selects 0 instances"));
```
##########
client/src/main/java/org/apache/cassandra/sidecar/client/RequestExecutor.java:
##########
@@ -305,26 +267,21 @@ private void
applyRetryPolicy(CompletableFuture<HttpResponse> future,
context.retryPolicy()
.onResponse(future, request, response, throwable, attempt,
retryOnNewHost, (nextAttempt, delay) -> {
String statusCode = response != null ?
String.valueOf(response.statusCode()) : "<Not Available>";
+ SidecarInstance nextInstance = sidecarInstance;
Review Comment:
This can be simplified a bit more:
```suggestion
SidecarInstance nextInstance = iterator.hasNext()
? iterator.next()
: sidecarInstance;
if (response == null || response.statusCode() !=
HttpResponseStatus.ACCEPTED.code())
{
logger.warn("Retrying stream on {} instance after {}ms. " +
"Failed on instance={}, attempt={},
statusCode={}",
nextInstance == sidecarInstance ? "same" :
"next", delay,
sidecarInstance, attempt, statusCode, throwable);
}
schedule(delay, () -> streamWithRetries(future, consumer,
iterator, nextInstance, context, nextAttempt));
```
##########
client/src/main/java/org/apache/cassandra/sidecar/client/RequestExecutor.java:
##########
@@ -142,62 +160,6 @@ public void close() throws Exception
httpClient.close();
}
- /**
- * Executes the request from the {@code context}, it iterates over the
{@link SidecarInstance}s until the response
- * satisfies the {@code retryPolicy}.
- *
- * @param future a future for the {@link HttpResponse}
- * @param iterator the iterator of instances
- * @param context the request context
- * @param attempt the number of attempts for this request
- * @param throwable the last {@link Throwable}, or {@code null} if there
are no previous errors
- */
- protected void executeWithRetries(CompletableFuture<HttpResponse> future,
- Iterator<SidecarInstance> iterator,
- RequestContext context,
- int attempt,
- Throwable throwable)
- {
- if (iterator.hasNext())
- {
- executeWithRetries(future, iterator, iterator.next(), context,
attempt);
- }
- else
- {
- // exhausted retries on all available hosts
- future.completeExceptionally(new
RetriesExhaustedException(attempt, context.request(), throwable));
- }
- }
Review Comment:
👍
--
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]
