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 2025/02/04 17:37:59 UTC
[PR] CASSSIDECAR-201: Make sidecar operations resilient to down Cassandra nodes [cassandra-sidecar]
yifan-c opened a new pull request, #186:
URL: https://github.com/apache/cassandra-sidecar/pull/186
This patch introduces InstanceMetadataFetcher#callOnFirstAvailableInstance to retry operations the next local Cassandra node until it is successful or exhausted. It also makes sure that host extracted from Host header is non-null to solidify the implementations.
Patch by Yifan Cai; Reviewed by TBD for CASSSIDECAR-201
--
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
Re: [PR] CASSSIDECAR-201: Make sidecar operations resilient to down Cassandra nodes [cassandra-sidecar]
Posted by "sarankk (via GitHub)" <gi...@apache.org>.
sarankk commented on code in PR #186:
URL: https://github.com/apache/cassandra-sidecar/pull/186#discussion_r1942345026
##########
server/src/main/java/org/apache/cassandra/sidecar/routes/AbstractHandler.java:
##########
@@ -290,22 +292,31 @@ private Name tableName(RoutingContext context, boolean required)
* Given a combined host address like 127.0.0.1:9042 or [2001:db8:0:0:0:ff00:42:8329]:9042, this method
* removes port information and returns 127.0.0.1 or 2001:db8:0:0:0:ff00:42:8329.
*
- * @param address ip address
+ * @param request http server request
* @return host address without port information
+ * @throws NoSuchCassandraInstanceException thrown when input address is null
*/
- public static String extractHostAddressWithoutPort(String address)
+ @NotNull
+ public static String extractHostAddressWithoutPort(HttpServerRequest request) throws NoSuchCassandraInstanceException
{
- if (address.contains(":"))
+ String host = request.host();
+ if (host == null)
+ {
+ throw new NoSuchCassandraInstanceException("No such Cassandra instance when Host header is absent");
Review Comment:
Nit: If host is not set in request, should we send back `Bad Request: Missing host header` instead of `404`
--
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
Re: [PR] CASSSIDECAR-201: Make sidecar operations resilient to down Cassandra nodes [cassandra-sidecar]
Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c commented on code in PR #186:
URL: https://github.com/apache/cassandra-sidecar/pull/186#discussion_r1943423471
##########
server/src/main/java/org/apache/cassandra/sidecar/utils/InstanceMetadataFetcher.java:
##########
@@ -77,60 +80,40 @@ public InstanceMetadata instance(int instanceId) throws NoSuchCassandraInstanceE
}
/**
- * Returns the {@link CassandraAdapterDelegate} for the given {@code host}. When the {@code host} is {@code null},
- * returns the delegate for the first instance from the list of configured instances.
+ * Iterate through the local instances and call the function on the first available instance
*
- * @param host the Cassandra instance hostname or IP address
- * @return the {@link CassandraAdapterDelegate} for the given {@code host}, or the first instance when {@code host}
- * is {@code null}
- * @throws NoSuchCassandraInstanceException when the Cassandra instance with {@code host} does not exist
- * @throws CassandraUnavailableException when Cassandra is not yet connected
+ * @param function function applies to {@link CassandraAdapterDelegate}
+ * @return function eval result. Null can be returned when all local instances are exhausted
+ * @param <T> type of the result
+ * @throws CassandraUnavailableException when all local instances are exhausted.
*/
@NotNull
- public CassandraAdapterDelegate delegate(@Nullable String host) throws NoSuchCassandraInstanceException, CassandraUnavailableException
Review Comment:
ok. I will add this specific method back, since both of you want it back.
In my opinion, retrieving delegate is not the responsibility of `InstanceMetadataFetcher`. It does not add a lot of character to retrieve from instanceMetadata. However, the methods have been there before this patch, probably could be exempted.
--
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
Re: [PR] CASSSIDECAR-201: Make sidecar operations resilient to down Cassandra nodes [cassandra-sidecar]
Posted by "sarankk (via GitHub)" <gi...@apache.org>.
sarankk commented on code in PR #186:
URL: https://github.com/apache/cassandra-sidecar/pull/186#discussion_r1942377373
##########
server/src/main/java/org/apache/cassandra/sidecar/utils/InstanceMetadataFetcher.java:
##########
@@ -77,60 +80,40 @@ public InstanceMetadata instance(int instanceId) throws NoSuchCassandraInstanceE
}
/**
- * Returns the {@link CassandraAdapterDelegate} for the given {@code host}. When the {@code host} is {@code null},
- * returns the delegate for the first instance from the list of configured instances.
+ * Iterate through the local instances and call the function on the first available instance
*
- * @param host the Cassandra instance hostname or IP address
- * @return the {@link CassandraAdapterDelegate} for the given {@code host}, or the first instance when {@code host}
- * is {@code null}
- * @throws NoSuchCassandraInstanceException when the Cassandra instance with {@code host} does not exist
- * @throws CassandraUnavailableException when Cassandra is not yet connected
+ * @param function function applies to {@link CassandraAdapterDelegate}
+ * @return function eval result. Null can be returned when all local instances are exhausted
+ * @param <T> type of the result
+ * @throws CassandraUnavailableException when all local instances are exhausted.
*/
@NotNull
- public CassandraAdapterDelegate delegate(@Nullable String host) throws NoSuchCassandraInstanceException, CassandraUnavailableException
Review Comment:
+1 since we seem to access `CassandraAdaptorDelegate` in many handlers shall we keep `delegate()`?
--
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
Re: [PR] CASSSIDECAR-201: Make sidecar operations resilient to down Cassandra nodes [cassandra-sidecar]
Posted by "frankgh (via GitHub)" <gi...@apache.org>.
frankgh commented on code in PR #186:
URL: https://github.com/apache/cassandra-sidecar/pull/186#discussion_r1942055612
##########
server/src/test/java/org/apache/cassandra/sidecar/coordination/ClusterLeaseClaimTaskTest.java:
##########
@@ -93,6 +93,22 @@ void testShouldNotSkipForMemberOfTheElectorate()
verify(mockElectorateMembership, times(1)).isMember();
}
+ @Test
+ void testShouldRescheduleWhenDatabaseAccessorIsUnavailable()
+ {
+ ServiceConfiguration serviceConfiguration = mockConfiguration(true, true);
Review Comment:
nice test
##########
server/src/main/java/org/apache/cassandra/sidecar/utils/InstanceMetadataFetcher.java:
##########
@@ -77,60 +80,40 @@ public InstanceMetadata instance(int instanceId) throws NoSuchCassandraInstanceE
}
/**
- * Returns the {@link CassandraAdapterDelegate} for the given {@code host}. When the {@code host} is {@code null},
- * returns the delegate for the first instance from the list of configured instances.
+ * Iterate through the local instances and call the function on the first available instance
*
- * @param host the Cassandra instance hostname or IP address
- * @return the {@link CassandraAdapterDelegate} for the given {@code host}, or the first instance when {@code host}
- * is {@code null}
- * @throws NoSuchCassandraInstanceException when the Cassandra instance with {@code host} does not exist
- * @throws CassandraUnavailableException when Cassandra is not yet connected
+ * @param function function applies to {@link CassandraAdapterDelegate}
+ * @return function eval result. Null can be returned when all local instances are exhausted
+ * @param <T> type of the result
+ * @throws CassandraUnavailableException when all local instances are exhausted.
*/
@NotNull
- public CassandraAdapterDelegate delegate(@Nullable String host) throws NoSuchCassandraInstanceException, CassandraUnavailableException
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
Re: [PR] CASSSIDECAR-201: Make sidecar operations resilient to down Cassandra nodes [cassandra-sidecar]
Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c commented on code in PR #186:
URL: https://github.com/apache/cassandra-sidecar/pull/186#discussion_r1941736080
##########
server/src/main/java/org/apache/cassandra/sidecar/routes/AbstractHandler.java:
##########
@@ -290,22 +292,31 @@ private Name tableName(RoutingContext context, boolean required)
* Given a combined host address like 127.0.0.1:9042 or [2001:db8:0:0:0:ff00:42:8329]:9042, this method
* removes port information and returns 127.0.0.1 or 2001:db8:0:0:0:ff00:42:8329.
*
- * @param address ip address
+ * @param request http server request
* @return host address without port information
+ * @throws NoSuchCassandraInstanceException thrown when input address is null
*/
- public static String extractHostAddressWithoutPort(String address)
Review Comment:
The original input `address` is nullable. But it is not handled. So the existing behavior is throwing NPE, leading to 503 internal server error. It is unexpected and no client side logic relies on this behavior.
In this patch, `null` value is handled, and server returns 404 not found.
Regardless of the handling change, the `handlers` are only going to see non-null `host` value, in both cases. The fact is clarified with adding the `@NotNull` annotation.
--
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
Re: [PR] CASSSIDECAR-201: Make sidecar operations resilient to down Cassandra nodes [cassandra-sidecar]
Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c commented on code in PR #186:
URL: https://github.com/apache/cassandra-sidecar/pull/186#discussion_r1943417072
##########
server/src/main/java/org/apache/cassandra/sidecar/routes/AbstractHandler.java:
##########
@@ -290,22 +292,31 @@ private Name tableName(RoutingContext context, boolean required)
* Given a combined host address like 127.0.0.1:9042 or [2001:db8:0:0:0:ff00:42:8329]:9042, this method
* removes port information and returns 127.0.0.1 or 2001:db8:0:0:0:ff00:42:8329.
*
- * @param address ip address
+ * @param request http server request
* @return host address without port information
+ * @throws NoSuchCassandraInstanceException thrown when input address is null
*/
- public static String extractHostAddressWithoutPort(String address)
+ @NotNull
+ public static String extractHostAddressWithoutPort(HttpServerRequest request) throws NoSuchCassandraInstanceException
{
- if (address.contains(":"))
+ String host = request.host();
+ if (host == null)
+ {
+ throw new NoSuchCassandraInstanceException("No such Cassandra instance when Host header is absent");
Review Comment:
+10000
Very good point
--
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
Re: [PR] CASSSIDECAR-201: Make sidecar operations resilient to down Cassandra nodes [cassandra-sidecar]
Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c merged PR #186:
URL: https://github.com/apache/cassandra-sidecar/pull/186
--
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
Re: [PR] CASSSIDECAR-201: Make sidecar operations resilient to down Cassandra nodes [cassandra-sidecar]
Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c commented on code in PR #186:
URL: https://github.com/apache/cassandra-sidecar/pull/186#discussion_r1941731693
##########
server/src/main/java/org/apache/cassandra/sidecar/acl/authorization/AuthorizationParameterValidateHandler.java:
##########
@@ -61,7 +62,7 @@ protected AuthorizationParameterValidateHandler(InstanceMetadataFetcher metadata
@Override
protected void handleInternal(RoutingContext context,
HttpServerRequest httpRequest,
- String host,
+ @NotNull String host,
Review Comment:
`host` is always non-null. Adding the annotation to clarify
--
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