You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "yifan-c (via GitHub)" <gi...@apache.org> on 2023/07/13 16:54:13 UTC
[GitHub] [cassandra-sidecar] yifan-c opened a new pull request, #61: Enrich RetriesExhaustedException to have more information for better visibility
yifan-c opened a new pull request, #61:
URL: https://github.com/apache/cassandra-sidecar/pull/61
(no 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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-sidecar] yifan-c commented on a diff in pull request #61: Enrich RetriesExhaustedException to have more information for better visibility
Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c commented on code in PR #61:
URL: https://github.com/apache/cassandra-sidecar/pull/61#discussion_r1262835413
##########
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);
}
+ SidecarInstance instance = nextInstance;
+ schedule(delay, () -> executeWithRetries(future, iterator, instance, context, nextAttempt));
Review Comment:
reviewer note: Same effect as the original implementation, but simplified.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-sidecar] yifan-c commented on a diff in pull request #61: Enrich RetriesExhaustedException to have more information for better visibility
Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c commented on code in PR #61:
URL: https://github.com/apache/cassandra-sidecar/pull/61#discussion_r1263980145
##########
src/test/integration/org/apache/cassandra/sidecar/routes/SnapshotsHandler2IntegrationTest.java:
##########
@@ -0,0 +1,143 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.sidecar.routes;
+
+import java.nio.file.Path;
+import java.util.List;
+import java.util.UUID;
+import java.util.concurrent.TimeUnit;
+
+import org.junit.jupiter.api.extension.ExtendWith;
+
+import com.datastax.driver.core.Session;
+import io.vertx.ext.web.client.WebClient;
+import io.vertx.ext.web.client.predicate.ResponsePredicate;
+import io.vertx.junit5.VertxExtension;
+import io.vertx.junit5.VertxTestContext;
+import org.apache.cassandra.sidecar.IntegrationTestBase;
+import org.apache.cassandra.sidecar.common.data.QualifiedTableName;
+import org.apache.cassandra.sidecar.common.testing.CassandraIntegrationTest;
+import org.apache.cassandra.sidecar.common.testing.CassandraTestContext;
+
+import static io.netty.handler.codec.http.HttpResponseStatus.OK;
+import static org.assertj.core.api.Assertions.assertThat;
+
+@ExtendWith(VertxExtension.class)
+class SnapshotsHandler2IntegrationTest extends IntegrationTestBase
Review Comment:
The commit is **not** to be merged.
I was just testing with CI. There is memory leak in the in-jvm dtest. The integration test gets killed when there are too many test cases and OOM'd
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-sidecar] yifan-c commented on a diff in pull request #61: Enrich RetriesExhaustedException to have more information for better visibility
Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c commented on code in PR #61:
URL: https://github.com/apache/cassandra-sidecar/pull/61#discussion_r1263068193
##########
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:
I feel those static creation methods make the code look nicer by eliminating the need to explicitly pass null.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-sidecar] yifan-c commented on a diff in pull request #61: Enrich RetriesExhaustedException to have more information for better visibility
Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c commented on code in PR #61:
URL: https://github.com/apache/cassandra-sidecar/pull/61#discussion_r1263063615
##########
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:
Yeah. For consistency, `sidecarInstance` fits better and I will update.
I called it "server" because the meaning of instance is vague.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-sidecar] yifan-c commented on a diff in pull request #61: Enrich RetriesExhaustedException to have more information for better visibility
Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c commented on code in PR #61:
URL: https://github.com/apache/cassandra-sidecar/pull/61#discussion_r1263063956
##########
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:
good call.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-sidecar] yifan-c commented on a diff in pull request #61: Enrich RetriesExhaustedException to have more information for better visibility
Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c commented on code in PR #61:
URL: https://github.com/apache/cassandra-sidecar/pull/61#discussion_r1262840617
##########
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);
}
/**
* 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 throwable the underlying exception
+ * @param attempts the number of attempts performed for the request
+ * @param request the HTTP request
+ * @param lastResponse the last failed HTTP response
+ * @param throwable the underlying exception
*/
- public RetriesExhaustedException(int attempts, Request request, Throwable throwable)
+ public static RetriesExhaustedException of(int attempts,
+ Request request,
+ HttpResponse lastResponse,
+ Throwable throwable)
{
- super(String.format("Unable to complete request '%s' after %d attempt%s",
- request.requestURI(), attempts, attempts == 1 ? "" : "s"), throwable);
+ return new RetriesExhaustedException(attempts, request, lastResponse, throwable);
+ }
+
+ /**
+ * 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 lastResponse the last failed HTTP response
+ * @param throwable the underlying exception
+ */
+ protected RetriesExhaustedException(int attempts,
Review Comment:
review note: There is no need to create multiple constructor overloads.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-sidecar] frankgh commented on a diff in pull request #61: Enrich RetriesExhaustedException to have more information for better visibility
Posted by "frankgh (via GitHub)" <gi...@apache.org>.
frankgh commented on code in PR #61:
URL: https://github.com/apache/cassandra-sidecar/pull/61#discussion_r1263831232
##########
src/test/integration/org/apache/cassandra/sidecar/routes/SnapshotsHandler2IntegrationTest.java:
##########
@@ -0,0 +1,143 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.sidecar.routes;
+
+import java.nio.file.Path;
+import java.util.List;
+import java.util.UUID;
+import java.util.concurrent.TimeUnit;
+
+import org.junit.jupiter.api.extension.ExtendWith;
+
+import com.datastax.driver.core.Session;
+import io.vertx.ext.web.client.WebClient;
+import io.vertx.ext.web.client.predicate.ResponsePredicate;
+import io.vertx.junit5.VertxExtension;
+import io.vertx.junit5.VertxTestContext;
+import org.apache.cassandra.sidecar.IntegrationTestBase;
+import org.apache.cassandra.sidecar.common.data.QualifiedTableName;
+import org.apache.cassandra.sidecar.common.testing.CassandraIntegrationTest;
+import org.apache.cassandra.sidecar.common.testing.CassandraTestContext;
+
+import static io.netty.handler.codec.http.HttpResponseStatus.OK;
+import static org.assertj.core.api.Assertions.assertThat;
+
+@ExtendWith(VertxExtension.class)
+class SnapshotsHandler2IntegrationTest extends IntegrationTestBase
Review Comment:
I'm not sure I understand this change
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-sidecar] yifan-c commented on a diff in pull request #61: Enrich RetriesExhaustedException to have more information for better visibility
Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c commented on code in PR #61:
URL: https://github.com/apache/cassandra-sidecar/pull/61#discussion_r1262835413
##########
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);
}
+ SidecarInstance instance = nextInstance;
+ schedule(delay, () -> executeWithRetries(future, iterator, instance, context, nextAttempt));
Review Comment:
Same effect as the original implementation, but simplified.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-sidecar] frankgh commented on a diff in pull request #61: Enrich RetriesExhaustedException to have more information for better visibility
Posted by "frankgh (via GitHub)" <gi...@apache.org>.
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-sidecar] yifan-c commented on a diff in pull request #61: Enrich RetriesExhaustedException to have more information for better visibility
Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c commented on code in PR #61:
URL: https://github.com/apache/cassandra-sidecar/pull/61#discussion_r1262839024
##########
client/src/main/java/org/apache/cassandra/sidecar/client/retry/RetryPolicy.java:
##########
@@ -55,31 +54,6 @@ public abstract void onResponse(CompletableFuture<HttpResponse> responseFuture,
boolean canRetryOnADifferentHost,
RetryAction retryAction);
- /**
- * Returns a {@link RetriesExhaustedException} with the number of {@code attempts} performed before the retries.
- *
- * @param attempts the number of attempts for the request
- * @param request the HTTP request
- * @return a {@link RetriesExhaustedException} with the number of {@code attempts} performed before the retries
- */
- RetriesExhaustedException retriesExhausted(int attempts, Request request)
- {
- return new RetriesExhaustedException(attempts, request);
- }
-
- /**
- * Returns a {@link RetriesExhaustedException} with the number of {@code attempts} performed before the retries.
- *
- * @param attempts the number of attempts for the request
- * @param request the HTTP request
- * @param throwable the underlying exception
- * @return a {@link RetriesExhaustedException} with the number of {@code attempts} performed before the retries
- */
- RetriesExhaustedException retriesExhausted(int attempts, Request request, Throwable throwable)
- {
- return new RetriesExhaustedException(attempts, request, throwable);
- }
-
Review Comment:
The exception creation methods are more helpful to be publicly accessible. They should not be limited to the RetryPolicy and its subclasses. Additionally, it is a lot of javadoc copy-and-paste by having creation method in the constructors and here.
##########
client/src/main/java/org/apache/cassandra/sidecar/client/retry/RetryPolicy.java:
##########
@@ -55,31 +54,6 @@ public abstract void onResponse(CompletableFuture<HttpResponse> responseFuture,
boolean canRetryOnADifferentHost,
RetryAction retryAction);
- /**
- * Returns a {@link RetriesExhaustedException} with the number of {@code attempts} performed before the retries.
- *
- * @param attempts the number of attempts for the request
- * @param request the HTTP request
- * @return a {@link RetriesExhaustedException} with the number of {@code attempts} performed before the retries
- */
- RetriesExhaustedException retriesExhausted(int attempts, Request request)
- {
- return new RetriesExhaustedException(attempts, request);
- }
-
- /**
- * Returns a {@link RetriesExhaustedException} with the number of {@code attempts} performed before the retries.
- *
- * @param attempts the number of attempts for the request
- * @param request the HTTP request
- * @param throwable the underlying exception
- * @return a {@link RetriesExhaustedException} with the number of {@code attempts} performed before the retries
- */
- RetriesExhaustedException retriesExhausted(int attempts, Request request, Throwable throwable)
- {
- return new RetriesExhaustedException(attempts, request, throwable);
- }
-
Review Comment:
reviewer note: The exception creation methods are more helpful to be publicly accessible. They should not be limited to the RetryPolicy and its subclasses. Additionally, it is a lot of javadoc copy-and-paste by having creation method in the constructors and here.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-sidecar] yifan-c closed pull request #61: Enrich RetriesExhaustedException to have more information for better visibility
Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c closed pull request #61: Enrich RetriesExhaustedException to have more information for better visibility
URL: https://github.com/apache/cassandra-sidecar/pull/61
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-sidecar] frankgh commented on a diff in pull request #61: Enrich RetriesExhaustedException to have more information for better visibility
Posted by "frankgh (via GitHub)" <gi...@apache.org>.
frankgh commented on code in PR #61:
URL: https://github.com/apache/cassandra-sidecar/pull/61#discussion_r1263110770
##########
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:
I mean, it's one additional call in the stack, but it might get optimized by the jvm.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-sidecar] yifan-c commented on a diff in pull request #61: Enrich RetriesExhaustedException to have more information for better visibility
Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c commented on code in PR #61:
URL: https://github.com/apache/cassandra-sidecar/pull/61#discussion_r1262834808
##########
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:
reviewer note: I found this method being redundant. It is only called when `iterator.hasNext()` is true, since the condition has been checked on the only call-site. The else branch is never hit. Then, the method has the same effect as its overload. Therefore, I am removing this method and the similar one for stream, for code simplicity.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-sidecar] yifan-c commented on a diff in pull request #61: Enrich RetriesExhaustedException to have more information for better visibility
Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c commented on code in PR #61:
URL: https://github.com/apache/cassandra-sidecar/pull/61#discussion_r1263068663
##########
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:
good catch
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org