CASSSIDECAR-173: Endpoint to retrieve C* gossip status #159

skoppu22 · 2024-12-10T15:21:45Z

https://issues.apache.org/jira/browse/CASSSIDECAR-173
Developing a new Sidecar endpoint to retrieve gossip running status of Cassandra, so the end user can use this endpoint without having to invoke C* JMX call. This endpoint will have authorization checks enabled once authorization feature is added to the Sidecar.

Circle CI run: https://app.circleci.com/pipelines/github/skoppu22/cassandra-sidecar?branch=gossips2

bbotella · 2024-12-10T22:44:40Z

...-common/src/main/java/org/apache/cassandra/sidecar/common/response/GossipStatusResponse.java

+        return gossipRunning;
+    }
+
+    private final boolean gossipRunning;


Define this at the beginning of the class

Most of Sidecar request/response classes following this style. I am also used to the style of defining at the beginning of the class, but just trying to be inline with existing classes. May be we can start changing the style now, as easy to find all variables right at the beginning of the class.

Most of Sidecar request/response classes following this style

This is not true, I'm not sure which class you are looking at, but all classes under client-common/src/main/java/org/apache/cassandra/sidecar/common/response define fields at the beginning of the class. And we should follow the Cassandra code structure https://cassandra.apache.org/_/development/code_style.html under Code Structure section > Class Layout.

This one I followed https://github.com/apache/cassandra-sidecar/blob/trunk/client-common/src/main/java/org/apache/cassandra/sidecar/common/response/ConnectedClientStatsResponse.java

This is something we should fix

Can it be enforced with code style? We want to layout the class in this order.

// variables public static protected static private static public protected private // methods // same ordering goes again // visible for testing methods // then, inner class

bbotella · 2024-12-10T22:50:32Z

server/src/main/java/org/apache/cassandra/sidecar/server/MainModule.java

@@ -380,6 +382,9 @@ public Router vertxRouter(Vertx vertx,
        router.get(ApiEndpointsV1.GOSSIP_INFO_ROUTE)
              .handler(gossipInfoHandler);

+        router.get(ApiEndpointsV1.GOSSIP_STATUS_ROUTE)


It is really unfortunate having to add an entire new API when we already have the GOSSIP_INFO_ROUTE endpoint? I even see that the GossipInfoResponse already has a deprecated STATUS field.

I understand that we are trying to make this call more efficient by only checking the gossip status. I wonder, could we add a filtering parameter that helps us only executing the part of the handler that we want?

I'm thinking of a filtering pattern like:
/api/v1/cassandra/gossip?filter=['status'] or something along those lines?

That way, and finding out why/where it was deprecated, maybe we can hijack it to use this storageOperations::isGossipRunning can save us from having to add this new API

?filter=<> doesn't sound right in this case, as filter is usually used to chose some/partial fields of the output which was returned without using filter. In this case /gossip endpoint doesn't return 'status', so we can't filter status from it. Let me see what are alternatives and will get back to you.

As we discussed offline, we cannot infer gossip running status from STATUS (or STATUS_WITH_PORT) fields of gossip info. Hence we cannot use gossip/filter=status for gossip running status, as that filter can be mistaken for STATUS field of gossip info. Will continue with gossip/status endpoint (Other option could be gossip/runningStatus, but sounds too verbose)

frankgh

Added some comments to the PR

frankgh · 2024-12-13T05:51:01Z

...-common/src/main/java/org/apache/cassandra/sidecar/common/response/GossipStatusResponse.java

+        return gossipRunning;
+    }
+
+    private final boolean gossipRunning;


Most of Sidecar request/response classes following this style

This is not true, I'm not sure which class you are looking at, but all classes under client-common/src/main/java/org/apache/cassandra/sidecar/common/response define fields at the beginning of the class. And we should follow the Cassandra code structure https://cassandra.apache.org/_/development/code_style.html under Code Structure section > Class Layout.

frankgh · 2024-12-13T05:52:32Z

...-common/src/main/java/org/apache/cassandra/sidecar/common/response/GossipStatusResponse.java

+    @JsonProperty("gossipRunning")
+    public boolean gossipRunning()
+    {
+        return gossipRunning;


returning a boolean becomes inflexible if you need to support a use case where you have more than one value. I suggest we change this to a payload that looks like

{ "status": "RUNNING" }

and

{ "status": "NOT_RUNNING" }

Client need to rely on deterministic values to parse the response and take actions accordingly. String "RUNNING" or "NOT_RUNNING" can break the client even with any minor changes or extra spaces etc in the output. Instead, if it is possible to return an enum as rest API response, we can use an enum. Otherwise boolean value true/false would be deterministic for the caller to parse the output

Well, both enum or boolean flags are technically strings that need to be parsed/interpreted by the client as well. I do like the idea @frankgh is proposing of having the enum with two possible states, that allows us to easily expand the amount of information we can provide.

Why boolean is treated as a string? Client can prepare GossipStatusResponse from the json received in the response and can get boolean using GossipStatusResponse.gossipRunning()

I meant that a response payload is a bunch of bytes that need to be parsed anyway, even if it is a json payload, it needs to be parsed. The client is in charge of parsing it and interpreting fields as booleans, strings, or anything else.

That said, I do agree with @frankgh in which the flexibility added by returning that enum is preferable in this case. On the client side, it is client's responsibility to parse the response correctly. In our current implementation, we control de java client for instance. We are also adding OpenAPI documentation that will help clients parse the given responses as part of this PR.

Long story short, I don't think returning a "RUNNING" response is making things harder for the client than returning a true, and it gives us flexibility to grow the API in the future.

Updated to String

frankgh · 2024-12-13T05:53:24Z

client/src/main/java/org/apache/cassandra/sidecar/client/SidecarClient.java

+     */
+    public CompletableFuture<GossipStatusResponse> gossipStatus()
+    {
+        return executor.executeRequestAsync(requestBuilder().gossipStatusRequest().build());


this request should be instance-specific

Thanks for noticing this

frankgh · 2024-12-13T05:54:18Z

server-common/src/main/java/org/apache/cassandra/sidecar/common/server/StorageOperations.java

+    /**
+     * @return returns gossip status
+     */
+    GossipStatusResponse isGossipRunning();


Maybe better aligned with the action we are trying to perform

Suggested change

GossipStatusResponse isGossipRunning();

GossipStatusResponse gossipStatus();

frankgh · 2024-12-13T06:12:02Z

server/src/main/java/org/apache/cassandra/sidecar/routes/AbstractHandler.java

+    protected StorageOperations getStorageOperations(String host)
+    {
+        CassandraAdapterDelegate delegate = this.metadataFetcher.delegate(host);
+        StorageOperations storageOperations = delegate == null ? null : delegate.storageOperations();
+        if (storageOperations == null)
+        {
+            throw cassandraServiceUnavailable();
+        }
+
+        return storageOperations;
+    }


we already have a method that does something similar in this class, we should just generalize that method. I suggest renaming ifMetricsOpsAvailable , and the code would look like this instead

protected <P> void ifAvailableFromDelegate(RoutingContext context, String host, Function<CassandraAdapterDelegate, P> mapper, Consumer<P> ifAvailable) { CassandraAdapterDelegate delegate = metadataFetcher.delegate(host); if (delegate == null) { context.fail(cassandraServiceUnavailable()); return; } P applied = mapper.apply(delegate); if (applied == null) { context.fail(cassandraServiceUnavailable()); return; } ifAvailable.accept(applied); }

The existing callsite would change to this:

ifAvailableFromDelegate(context, host, CassandraAdapterDelegate::metricsOperations, operations -> {

Looks like this is no longer needed as you moved null checks into getters

frankgh · 2024-12-13T06:14:02Z

server/src/main/java/org/apache/cassandra/sidecar/routes/GossipStatusHandler.java

+                               Void request)
+    {
+        StorageOperations storageOperations = getStorageOperations(host);
+        logger.debug("Retrieving gossip status, remoteAddress={}, instance={}",


this log entry is redundant, we already log

logger.debug("{} received request={}, remoteAddress={}, instance={}", this.getClass().getSimpleName(), requestParams, remoteAddress, host);

for all requests in handlers of the AbstractHandler type.

frankgh · 2024-12-13T06:16:49Z

...test/integration/org/apache/cassandra/sidecar/routes/GossipStatusHandlerIntegrationTest.java

+    private static final String testRoute = "/api/v1/cassandra/gossip/status";
+
+    @CassandraIntegrationTest()
+    void testGossipDisabled(CassandraTestContext context, VertxTestContext testContext)


can we combine both test cases into a single test case? these are expensive to spin up, so whenever possible we should reuse the in-jvm dtest apparatus available .

frankgh · 2024-12-13T06:19:31Z

server/src/test/java/org/apache/cassandra/sidecar/routes/JmxCommonTest.java

+ */
+class JmxCommonTest
+{
+    static final Logger LOGGER = LoggerFactory.getLogger(JmxCommonTest.class);


if we already have an in-jvm dtest this becomes less useful IMO. There is no need to do mocking if we have the actual server running

We do more test scenarios using mock, especially related to failure cases as mock makes it easy to throw exceptions or return invalid values

Can we get code coverage measured with in-jvm dtests? For example, someone using open source Sidecar and hooking to their build systems, can they get code coverage numbers with in-jvm dtests?

frankgh

Looking good, I have a few additional comments.

frankgh · 2025-01-09T20:40:06Z

...-common/src/main/java/org/apache/cassandra/sidecar/common/response/GossipStatusResponse.java

+     * Constructs a response with gossip status
+     * @param gossipRunningStatus running status of gossip
+     */
+    public GossipStatusResponse(@JsonProperty("gossipRunningStatus") String gossipRunningStatus)


I think status should be sufficient. We don't have a precedent for qualifying the field name like this.

Just calling it 'status' can be mistaken with STATUS field of gossip info, as we discussed before regarding this field. Gossip running status just tells is gossip thread running in C*. Where as STATUS field has different meaning. So we need to be clear that this is gossip thread running status

frankgh · 2025-01-09T20:42:30Z

...-common/src/main/java/org/apache/cassandra/sidecar/common/response/GossipStatusResponse.java

+    }
+
+    @JsonProperty("gossipRunningStatus")
+    public String gossipRunningStatus()


should we add a utility method here isRunning that returns "RUNNING".equals(gossipRunningStatus). Similar to what you had in an earlier iteration of this PR.

frankgh · 2025-01-09T20:43:01Z

server-common/src/main/java/org/apache/cassandra/sidecar/common/server/StorageOperations.java

@@ -102,4 +103,9 @@ default void outOfRangeDataCleanup(@NotNull String keyspace, @NotNull String tab
    {
        outOfRangeDataCleanup(keyspace, table, 1);
    }
+
+    /**
+     * @return returns gossip status


Suggested change

* @return returns gossip status

* @return the gossip status

frankgh · 2025-01-09T20:43:41Z

server/src/main/java/org/apache/cassandra/sidecar/routes/GossipStatusHandler.java

+
+
+/**
+ * Handler for to retrieve gossip status


Suggested change

* Handler for to retrieve gossip status

* Handles gossip status retrieval

frankgh · 2025-01-09T20:45:04Z

...test/integration/org/apache/cassandra/sidecar/routes/GossipStatusHandlerIntegrationTest.java

+    }
+
+    @CassandraIntegrationTest()
+    void testGossipStatus(CassandraTestContext context, VertxTestContext testContext)


frankgh · 2025-01-09T20:47:08Z

server/src/test/java/org/apache/cassandra/sidecar/routes/GossipStatusHandlerTest.java

+    private static final String testRoute = "/api/v1/cassandra/gossip/status";
+
+    @Test
+    void testGossipRunning(VertxTestContext context)


this test is redundant, not sure what we gain here.

The gain depends on answer to https://github.com/apache/cassandra-sidecar/pull/159/files#r1886797448

Do we get code coverage with in-jvm tests? If yes, we can remove this test

frankgh · 2025-01-09T20:47:43Z

server/src/test/java/org/apache/cassandra/sidecar/routes/GossipStatusHandlerTest.java

+    }
+
+    @Test
+    void testGossipNotRunning(VertxTestContext context)


this is also redundant not sure what exactly we are testing here.

frankgh · 2025-01-09T20:50:46Z

server/src/test/java/org/apache/cassandra/sidecar/routes/GossipStatusHandlerTest.java

+    }
+
+    @Test
+    void testWithInstanceId(VertxTestContext context)


also, this test doesn't make a lot of sense to me. My problem with the test is that in the setup JmxCommonTest you share the delegate and storageOperations among 3 instances. This setup is problematic because in the real execution of sidecar each instance will have its own delegate and all the operations associated with a single delegate. I would drop this test, we already cover accessing different instances with the query parameter, so I would consider testing this outside of the scope of this test.

frankgh · 2025-01-09T20:51:39Z

server/src/test/java/org/apache/cassandra/sidecar/routes/GossipStatusHandlerTest.java

+              }));
+    }
+
+    private void verifyValidResponse(VertxTestContext context, HttpResponse<Buffer> response, String expectedValue)


maybe call this verifyAndComplete

frankgh · 2025-01-09T20:55:50Z

server/src/test/java/org/apache/cassandra/sidecar/routes/JmxCommonTest.java

+        @Singleton
+        public InstancesMetadata instanceConfig()
+        {
+            int instanceId1 = 100;


I think we should probably model this correctly if we are meaning to use this in other tests.

We can remove multi instance stuff from this. But I see almost all tests in this directory are having module extending AbstractModule and using mocks, I believe we can move common code to a common test framework file and reuse in most of tests

yifan-c

Thanks for the contribution.

First of all, please add an entry to CHANGES.txt.

Second, the patch is to add an API to check whether gossip is enabled or disabled in the companion Cassandra node. There are existing health APIs that check the status of corresponding components. See the following code block for example.

GET /api/v1/cassandra/native/__health
{"status" : "OK | NOT_OK"}

GET /api/v1/cassandra/jmx/__health
{"status" : "OK | NOT_OK"}

I think we should be consistent with the existing pattern. In other words, the API should be this.

GET /api/v1/cassandra/gossip/__health
{"status" : "OK | NOT_OK"}

bbotella · 2025-01-09T23:34:13Z

...ase/src/main/java/org/apache/cassandra/sidecar/adapters/base/CassandraStorageOperations.java

+    public GossipStatusResponse gossipStatus()
+    {
+        return new GossipStatusResponse(jmxClient.proxy(StorageJmxOperations.class, STORAGE_SERVICE_OBJ_NAME)
+                                                 .isGossipRunning() ? "RUNNING" : "NOT_RUNNING");


What do you think about moving "RUNNING" and "NOT_RUNNING" to an enum?

I actually agree with @yifan-c suggestion. So, what about having the enum for the "OK" and "NOT_OK"?

frankgh · 2025-01-09T23:41:21Z

I think we should be consistent with the existing pattern. In other words, the API should be this.

I agree with Yifan, we should try to stick closer to the existing patterns. +1 on this:

GET /api/v1/cassandra/gossip/__health
{"status" : "OK | NOT_OK"}

skoppu22 · 2025-01-13T15:14:42Z

Thanks for the contribution.

First of all, please add an entry to CHANGES.txt.

Second, the patch is to add an API to check whether gossip is enabled or disabled in the companion Cassandra node. There are existing health APIs that check the status of corresponding components. See the following code block for example.
GET /api/v1/cassandra/native/__health
{"status" : "OK | NOT_OK"}

GET /api/v1/cassandra/jmx/__health
{"status" : "OK | NOT_OK"}
I think we should be consistent with the existing pattern. In other words, the API should be this.
GET /api/v1/cassandra/gossip/__health
{"status" : "OK | NOT_OK"}

This API is returning C* gossip thread running status (just tells is gossip thread running or not). Having thread running doesn't mean gossip health is OK. So I wouldn't prefer to call it 'gossip health'. Gossip thread may be running, but gossip state might be not healthy at C* side

skoppu22 force-pushed the gossips2 branch from 5925862 to 29d7068 Compare December 10, 2024 15:38

bbotella reviewed Dec 10, 2024

View reviewed changes

frankgh reviewed Dec 13, 2024

View reviewed changes

skoppu22 force-pushed the gossips2 branch from 35c34b0 to 37fb9e3 Compare December 16, 2024 15:46

Endpoint to retrieve C* gossip status

f2ed5c7

skoppu22 force-pushed the gossips2 branch from fb21334 to f2ed5c7 Compare January 9, 2025 15:30

resolve 1

fb5606b

skoppu22 force-pushed the gossips2 branch from eaffd56 to fb5606b Compare January 9, 2025 17:39

frankgh reviewed Jan 9, 2025

View reviewed changes

yifan-c requested changes Jan 9, 2025

View reviewed changes

bbotella reviewed Jan 9, 2025

View reviewed changes

increase memory for tests

7fecf06

skoppu22 force-pushed the gossips2 branch from 820c85c to 7fecf06 Compare January 13, 2025 15:35

	GossipStatusResponse isGossipRunning();
	GossipStatusResponse gossipStatus();

	* Handler for to retrieve gossip status
	* Handles gossip status retrieval

CASSSIDECAR-173: Endpoint to retrieve C* gossip status #159

Are you sure you want to change the base?

CASSSIDECAR-173: Endpoint to retrieve C* gossip status #159

Conversation

skoppu22 commented Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skoppu22 Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbotella Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frankgh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frankgh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skoppu22 Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yifan-c left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frankgh commented Jan 9, 2025

skoppu22 commented Jan 13, 2025

skoppu22 commented Dec 10, 2024 •

edited

Loading

skoppu22 Dec 16, 2024 •

edited

Loading

bbotella Dec 10, 2024 •

edited

Loading

skoppu22 Jan 13, 2025 •

edited

Loading