CASSSIDECAR-360: Sidecar API Endpoint for Nodetool Compaction Stop #272

shalnisundram · 2025-10-30T17:19:53Z

Add bespoke API endpoint for nodetool compaction stop.

This implementation carries out functionality currently performed when execution command nodetool stop, which is a short-running operation that requests ongoing compactions of a specified type or id to be stopped.

See CASSANDRA-20021 JIRA ticket - more details can be found on ongoing effort to build APIs for nodetool C* cluster management operations.

patch by Shalni Sundram

bbotella · 2025-10-30T17:38:33Z

...in/java/org/apache/cassandra/sidecar/adapters/base/CassandraCompactionManagerOperations.java

+        else
+        {
+            // Handler guarantees compactionType is non-null/non-empty
+            proxy.stopCompaction(compactionType);


Maybe we should add a @NotNull annotation to compactionType?

See change on CassandraCompactionManagerOperations line 77 . Not exactly an annotation but a requireNonNull check. Let me know if that's acceptable.

client-common/src/main/java/org/apache/cassandra/sidecar/common/ApiEndpointsV1.java

...-common/src/main/java/org/apache/cassandra/sidecar/common/request/CompactionStopRequest.java

bbotella · 2025-10-30T17:41:30Z

...main/java/org/apache/cassandra/sidecar/common/request/data/CompactionStopRequestPayload.java

+@JsonInclude(JsonInclude.Include.NON_NULL)
+public class CompactionStopRequestPayload
+{
+    private final String compactionType;


Should this be an enum instead of a free string?

Chose deliberately to use String, as I figured this would encourage forward compatibility with future C* versions if more compactionTypes are added later. i.e., would want to avoid sidecar forcing all C* versions into a single enum definition, and would rather leave this responsibility to cassandra itself. Would appreciate @arjunashok's input for this though.

You are already defining the compaction types as a set in AbstractHandler.SUPPORTED_COMPACTION_TYPES. Better to make it an enum instead of a set.

...ommon/src/main/java/org/apache/cassandra/sidecar/common/response/CompactionStopResponse.java

server/src/main/java/org/apache/cassandra/sidecar/modules/CassandraOperationsModule.java

bbotella · 2025-10-30T17:46:19Z

server/src/main/java/org/apache/cassandra/sidecar/modules/CassandraOperationsModule.java

+    @APIResponse(description = "Compaction stop operation completed",
+            responseCode = "200",
+            content = @Content(mediaType = "application/json",
+                    schema = @Schema(implementation = CompactionStopResponse.class)))


Please add the @apiresponse for the error codes as well

See lines 149-189 of CompactionStopHandler.java - I do some custom error handling here, as particular api response codes will have different messages thrown depending on the input. Let me know if I should still edit this section in CassandraOperationsModule.

sklaha

Thank you for the PR. Looks good at a high level. Please run ./gradlew clean; ./gradlew build which will show you check style issues. I am leaving my other comments inline.

server/src/main/java/org/apache/cassandra/sidecar/handlers/CompactionStopHandler.java

...in/java/org/apache/cassandra/sidecar/adapters/base/CassandraCompactionManagerOperations.java

sklaha · 2025-11-05T05:33:05Z

...s/src/integrationTest/org/apache/cassandra/sidecar/routes/CompactionStopIntegrationTest.java

+    }
+
+    @Test
+    void testStopCompactionByIdSuccess()


In the current implementation, a request with a non-existent compaction ID silently succeeds. The JMX call simply succeeds without any action being taken. It would be more informative to update the Cassandra JMX implementation to return a boolean value or some other indicator. I will seek input from others on this.

https://github.pie.apple.com/slaha/aci-cassandra/blob/38ab7d02e0979074f3de1c7f90ce70668d2492b9/src/java/org/apache/cassandra/db/compaction/CompactionManager.java#L2306

...s/src/integrationTest/org/apache/cassandra/sidecar/routes/CompactionStopIntegrationTest.java

client-common/src/main/java/org/apache/cassandra/sidecar/common/ApiEndpointsV1.java

...-common/src/main/java/org/apache/cassandra/sidecar/common/request/CompactionStopRequest.java

...ommon/src/main/java/org/apache/cassandra/sidecar/common/response/CompactionStopResponse.java

arjunashok · 2025-11-18T21:34:52Z

...ommon/src/main/java/org/apache/cassandra/sidecar/common/response/CompactionStopResponse.java

+    public static final String STATUS_KEY = "status";
+    public static final String ERROR_CODE_KEY = "error_code";


Let's evaluate the intended usage of the error_code field in determining the values. Based on the current name, it seems to suggest this is a status code (currently returning 200 OK)

The 200 status code would be redundant in this case, as it is implied with the HTTP response code.

The other option is to do away with this field since we have a reason field to denote an arbitrary string reason when there is an error (or a non-200 response).

Also, can status be an enum here? Seems like it can be one of 3 or 4 values - SUBMITTED, PENDING or FAILED

Original status options for the command were just two - "PENDING" and "FAILED". For this API it's "SUBMITTED" (more clear I think) and "FAILED".
Status changed to enum - see CompactionStopStatus.java for new enum class

server/src/main/java/org/apache/cassandra/sidecar/modules/CassandraOperationsModule.java

arjunashok

Looking good for the most part. Added some minor comments.

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

arjunashok · 2025-12-02T01:14:44Z

...s/src/integrationTest/org/apache/cassandra/sidecar/routes/CompactionStopIntegrationTest.java

+        assertThat(stopResponse).isNotNull();
+        assertThat(stopResponse.status()).isEqualTo(CompactionStopStatus.SUBMITTED);
+        assertThat(stopResponse.compactionType()).isEqualTo("COMPACTION");
+        assertThat(stopResponse.reason()).isEqualTo("Operation Succeeded");


No longer applicable, correct?

Omitted the reason() field - the status , type, and id fields still stand in the response so left those in

...s/src/integrationTest/org/apache/cassandra/sidecar/routes/CompactionStopIntegrationTest.java

...in/java/org/apache/cassandra/sidecar/adapters/base/CassandraCompactionManagerOperations.java

bbotella · 2025-12-03T22:45:12Z

client-common/src/main/java/org/apache/cassandra/sidecar/common/ApiEndpointsV1.java

    public static final String STREAM_STATS_ROUTE = API_V1 + CASSANDRA + "/stats/streams";
    public static final String TABLE_STATS_ROUTE = API_V1 + CASSANDRA + PER_KEYSPACE + PER_TABLE + "/stats";
-
+    public static final String COMPACTION_STOP_ROUTE = API_V1 + CASSANDRA + "/operations/compaction/stop";


nit: I see the operations part of the path in several static variables here. Let's extract it to its own private static

Done. Added the private static, but only used it for the node stop route rather than node drain and node decommission routes which also uses this path, as those changes would not be directly related to this PR.

...main/java/org/apache/cassandra/sidecar/common/request/data/CompactionStopRequestPayload.java

bbotella · 2025-12-03T22:58:53Z

server/src/main/java/org/apache/cassandra/sidecar/handlers/CompactionStopHandler.java

+
+        // Attempt to stop the compaction
+        // If compactionId is provided, use it (takes precedence over type)
+        if (compactionId != null && !compactionId.trim().isEmpty()) {


I see this pattern of "if compactionID then do otherwise do by type" in several different places. What do you think about abstracting that inside operations, with a generic method that does stopCompaction(@Nullable String compactionId, @Nullable String compactionType), and performs that if internally?

Sorry if this has already been discussed.

Yeah in the latter case we would have a single method that accepts two params but only uses one, which others expressed was not preferable. Thus, preserved the stopByID() and stopCompaction() structure already used by node stop

bbotella · 2025-12-03T23:00:26Z

server/src/main/java/org/apache/cassandra/sidecar/handlers/CompactionStopHandler.java

+
+        // Validate that at least one field is provided
+        if (payload.compactionType() == null &&
+            (payload.compactionId() == null || payload.compactionId().trim().isEmpty()))


I also see this pattern of == null or == null or empty across different places. What about adding boolean public methods on payload that return if they are valid? Something like isCompactionTypeValid() and isCompactionIdValid?

Done - added those functions in CompactionStopRequestPayload.java, although didn't use those functions for the pattern present in CassandraCompactionManagerOperations.java, as those methods don't work with the payload object.

cassandra-sidecar/client-common/src/main/java/org/apache/cassandra/sidecar/common/request/data/CompactionStopRequestPayload.java

Line 77 in 0a9b3f4

public boolean hasValidCompactionId() {

...ava/org/apache/cassandra/sidecar/adapters/base/CassandraCompactionManagerOperationsTest.java

.../java/org/apache/cassandra/sidecar/common/request/data/CompactionStopRequestPayloadTest.java

server/src/main/java/org/apache/cassandra/sidecar/handlers/CompactionStopHandler.java

...in/java/org/apache/cassandra/sidecar/adapters/base/CassandraCompactionManagerOperations.java

jyothsnakonisa · 2025-12-16T22:49:21Z

Looks good overall, few minor comments

sarankk

+1 Thanks Shalni, overall looks great to me. Left some comments, I am +1 once the comments are addressed.

...in/java/org/apache/cassandra/sidecar/adapters/base/CassandraCompactionManagerOperations.java

...main/java/org/apache/cassandra/sidecar/adapters/base/jmx/CompactionManagerJmxOperations.java

client-common/src/main/java/org/apache/cassandra/sidecar/common/ApiEndpointsV1.java

...on/src/main/java/org/apache/cassandra/sidecar/common/server/CompactionManagerOperations.java

server/src/main/java/org/apache/cassandra/sidecar/modules/CassandraOperationsModule.java

...s/adapters-base/src/main/java/org/apache/cassandra/sidecar/adapters/base/CompactionType.java

client-common/src/main/java/org/apache/cassandra/sidecar/common/data/CompactionType.java

client-common/src/main/java/org/apache/cassandra/sidecar/common/data/CompactionStopStatus.java

sarankk

Changes look great to me, left final set of comments.

sarankk · 2026-01-07T19:19:20Z

...in/java/org/apache/cassandra/sidecar/adapters/base/CassandraCompactionManagerOperations.java

+    @Override
+    public List<String> supportedCompactionTypes()
+    {
+        return Arrays.stream(CompactionType.values())


Instead of creating a stream of values each time, can you compute it once and return it?

...adapters-base/src/main/java/org/apache/cassandra/sidecar/adapters/base/CassandraFactory.java

...s/adapters-base/src/main/java/org/apache/cassandra/sidecar/adapters/base/CompactionType.java

...ava/org/apache/cassandra/sidecar/adapters/base/CassandraCompactionManagerOperationsTest.java

...ra41/src/main/java/org/apache/cassandra/sidecar/adapters/cassandra41/Cassandra41Adapter.java

...on/src/main/java/org/apache/cassandra/sidecar/common/server/CompactionManagerOperations.java

...ommon/src/main/java/org/apache/cassandra/sidecar/common/response/CompactionStopResponse.java

server/src/main/java/org/apache/cassandra/sidecar/handlers/CompactionStopHandler.java

…detool compaction stop API implementation

…ts, fix bug in handler

…gration test

…ions for integration tests

…on stop

…est.java

…ported compaction type per cassandra version limit CompactionType enum to use in cassandra version-specific adapters to check for supported compaction type

sarankk · 2026-01-08T22:38:01Z

...adapters-base/src/main/java/org/apache/cassandra/sidecar/adapters/base/CassandraFactory.java


    public CassandraFactory(DnsResolver dnsResolver, DriverUtils driverUtils, TableSchemaFetcher tableSchemaFetcher)
    {
-        this.dnsResolver = Objects.requireNonNull(dnsResolver, "dnsResolver is required");


Can you revert these changes?

sarankk · 2026-01-08T22:40:20Z

...ra41/src/main/java/org/apache/cassandra/sidecar/adapters/cassandra41/Cassandra41Factory.java


    public Cassandra41Factory(DnsResolver dnsResolver, DriverUtils driverUtils, TableSchemaFetcher tableSchemaFetcher)
    {
-        this.dnsResolver = Objects.requireNonNull(dnsResolver, "dnsResolver is required");


Can you revert these changes, they seem to be unrelated to PR

sarankk · 2026-01-08T22:40:56Z

...ra41/src/main/java/org/apache/cassandra/sidecar/adapters/cassandra41/Cassandra41Adapter.java

    @Override
    @NotNull
-    protected StorageOperations createStorageOperations(DnsResolver dnsResolver, JmxClient jmxClient)
+    public StorageOperations storageOperations()


Can you revert this change too

bbotella reviewed Oct 30, 2025

View reviewed changes

sklaha reviewed Nov 5, 2025

View reviewed changes

arjunashok reviewed Nov 18, 2025

View reviewed changes

client-common/src/main/java/org/apache/cassandra/sidecar/common/ApiEndpointsV1.java Outdated Show resolved Hide resolved

arjunashok reviewed Nov 18, 2025

View reviewed changes

shalnisundram force-pushed the CASSSIDECAR-360 branch 4 times, most recently from fd5e624 to a6b5497 Compare November 19, 2025 19:06

arjunashok reviewed Dec 2, 2025

View reviewed changes

shalnisundram force-pushed the CASSSIDECAR-360 branch 5 times, most recently from def6e6f to f404939 Compare December 3, 2025 21:05

bbotella reviewed Dec 3, 2025

View reviewed changes

shalnisundram force-pushed the CASSSIDECAR-360 branch from b7d1bae to 39c9242 Compare December 8, 2025 22:04

jyothsnakonisa approved these changes Dec 16, 2025

View reviewed changes

sarankk reviewed Dec 21, 2025

View reviewed changes

sarankk reviewed Dec 22, 2025

View reviewed changes

client-common/src/main/java/org/apache/cassandra/sidecar/common/data/CompactionStopStatus.java Show resolved Hide resolved

sarankk reviewed Jan 7, 2026

View reviewed changes

shalnisundram force-pushed the CASSSIDECAR-360 branch from bddf663 to ef8b32c Compare January 8, 2026 18:25

Shalni Sundram added 7 commits January 8, 2026 13:16

implement API endpoint for nodetool compaction stop

9fa108b

include CassandraCompactionManagerOperations.java modification for no…

04992b3

…detool compaction stop API implementation

clean imports, add annotations, improve naming for constants

b73f81d

add compaction type enum support, add full compaction integration tes…

9283dc7

…ts, fix bug in handler

remove redundant JSON response fields for compaction stop endpoint

e5a2491

add SidecarClientTest test for compaction stop, assumeTrue() for inte…

bb66d4a

…gration test

make minor structural changes, adjust tests

02a2bcb

Shalni Sundram and others added 11 commits January 8, 2026 13:28

throttle compaction via nodetool compactiontrhoughput to slow compact…

516c6e6

…ions for integration tests

fix enum errors in integration tests

30b538d

shorten integration test duration for triggering and testing compacti…

7081253

…on stop

eliminiate redundant tests and settings in CompactionStopIntegrationT…

548c774

…est.java

return trimmed compaction ID, throw errors, add new lines

60dde56

change post to put request, clean up, edit compaction types

c88698f

resolve checksytle errors

7190a1b

apply formatting changes

cf1cbec

edit enums in tests, make bug fix for post to put request change

996e0e8

limit CompactionType functionality to adapter-specific checks for sup…

fd631a2

…ported compaction type per cassandra version limit CompactionType enum to use in cassandra version-specific adapters to check for supported compaction type

remove redundant CompactionType logic in cassandra41 adapter

1aeaa4f

shalnisundram force-pushed the CASSSIDECAR-360 branch from ef8b32c to 1aeaa4f Compare January 8, 2026 22:32

sarankk reviewed Jan 8, 2026

View reviewed changes

shalnisundram added 2 commits January 8, 2026 15:24

revert Cassandra41Adapter changes

85fb56f

add cassandra50 adapter to integration test framework

ddc6be8

		public static final String STATUS_KEY = "status";
		public static final String ERROR_CODE_KEY = "error_code";

CASSSIDECAR-360: Sidecar API Endpoint for Nodetool Compaction Stop #272

Are you sure you want to change the base?

CASSSIDECAR-360: Sidecar API Endpoint for Nodetool Compaction Stop #272

Uh oh!

Conversation

shalnisundram commented Oct 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shalnisundram Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sklaha left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shalnisundram Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

arjunashok left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jyothsnakonisa commented Dec 16, 2025

shalnisundram Oct 31, 2025 •

edited

Loading

shalnisundram Nov 19, 2025 •

edited

Loading