CASSSIDECAR-344: Sidecar endpoint for moving a node to a new token #275

sklaha · 2025-11-05T08:01:37Z

Sidecar endpoint for moving a node to a new token

These changes are part of the effort to introduce bespoke Sidecar APIs to support key operational functionality currently managed through nodetool commands. Changes in the commit:

API for moving a node to a new token
Unit tests
Integration tests

frankgh

Looks good in general, I have some comments.

frankgh · 2025-12-02T22:00:17Z

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

+     *
+     * @param newToken the new token for the node to move to
+     */
+    void move(String newToken);


We need to propagate any potential IOException from JMX and we should handle it

Suggested change

void move(String newToken);

void move(String newToken) throws IOException;

Updated the method signature. OperationalJob.execute() already handles the exception. I also added tests for node move failure.

frankgh · 2025-12-02T22:10:08Z

...pters-base/src/main/java/org/apache/cassandra/sidecar/adapters/base/utils/DataTypeUtils.java

+     * @param newToken the string to validate as a BigInteger, can be null
+     * @return true if the string can be successfully parsed as a BigInteger, false if null or invalid
+     */
+    public static boolean isValidBigInt(String newToken)


I would defer validation to Cassandra, and handle the exception thrown by Cassandra. There are tokens that are not long-based or big integer based

I think, OrderPreservingPartitioner is deprecated and token is numeric for all other partitioners. Please correct me if I am wrong.

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

frankgh · 2025-12-02T22:25:59Z

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

+        String body = context.body().asString();
+        if (body == null || body.equalsIgnoreCase("null"))
+        {
+            logger.warn("Bad request. Received null payload.");


a log entry will be logged by the parent class when we throw the exception, so I think we should remove this line:

Suggested change

logger.warn("Bad request. Received null payload.");

frankgh · 2025-12-02T22:28:58Z

...n/src/main/java/org/apache/cassandra/sidecar/common/request/data/NodeMoveRequestPayload.java

+    @JsonCreator
+    public NodeMoveRequestPayload(@JsonProperty(value = "newToken", required = true) String newToken)
+    {
+        this.newToken = newToken;


We should ensure the token is provided

Suggested change

this.newToken = newToken;

this.newToken = Objects.requireNonNull(newToken, "newToken is required");

frankgh · 2025-12-02T22:30:12Z

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

+        {
+            NodeMoveRequestPayload payload = Json.decodeValue(body, NodeMoveRequestPayload.class);
+            String newToken = payload.newToken();
+            if (StringUtils.isNullOrEmpty(newToken) || !DataTypeUtils.isValidBigInt(newToken))


let's defer validation of the token to Cassandra. Since some tokens might not necessarily be numeric, i.e. org.apache.cassandra.dht.OrderPreservingPartitioner

I think, OrderPreservingPartitioner is deprecated and token is numeric for all other partitioners. Please correct me if I am wrong.

yeah, that's correct, but I think we shouldn't make assumptions from the Sidecar side. Maybe reducing the input to BigInteger is sufficient, but I prefer to defer validation to the partitioner.

I updated the validation of the token.

frankgh · 2025-12-02T22:31:31Z

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

+        }
+        catch (DecodeException e)
+        {
+            logger.warn("Bad request. Received invalid JSON payload.");


This will be logged in the parent abstract class. No need to log it here

Suggested change

logger.warn("Bad request. Received invalid JSON payload.");

frankgh · 2025-12-02T22:36:37Z

server/src/main/java/org/apache/cassandra/sidecar/job/NodeMoveJob.java

+{
+    private static final Logger LOGGER = LoggerFactory.getLogger(NodeMoveJob.class);
+    private static final String OPERATION = "move";
+    private static final String OPERATION_MODE_MOVING = "MOVING";


do we need to consider the org.apache.cassandra.service.StorageService.Mode#MOVE_FAILED mode as well?

We just need to check if a move operation is in progress or not. Check if the status is "MOVING" accomplishes that.

frankgh · 2025-12-02T22:37:39Z

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

+                 responseCode = "200",
+                 content = @Content(mediaType = "application/json",
+                 schema = @Schema(implementation = OperationalJobResponse.class)))
+    @APIResponse(description = "Node move operation initiated successfully",


can we document the conflict scenario as well?

frankgh · 2025-12-02T22:39:46Z

...egrationTest/org/apache/cassandra/sidecar/routes/CassandraNodeOperationsIntegrationTest.java

+        assertThat(ring).isNotNull();
+
+        RingEntry ringEntry = ring.stream()
+                                  .filter(entry -> entry.fqdn().equals(node))


I think this won't work if we had vnodes enabled for this test.

I didn't enable vnodes for this test and it succeeds.

frankgh · 2025-12-22T18:19:11Z

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

+@Singleton
+public class NodeMoveHandler extends AbstractHandler<String> implements AccessProtected
+{
+    private static final int MAX_TOKEN_LENGTH = 128;


NIT, document how you came up with value 128 for the maximum token length

frankgh

+1 looks good to me

sarankk

+1 Looks good to me, some minor nits.

sarankk · 2025-12-23T02:42:55Z

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

+         * @param newToken the new token for the node to move to
+         * @return a reference to this Builder
+         */
+        public Builder nodeMoveRequest(String newToken)


Nit: @NotNull annotation for newToken

sarankk · 2025-12-23T02:47:50Z

client/src/testFixtures/java/org/apache/cassandra/sidecar/client/SidecarClientTest.java

+    {
+        UUID jobId = UUID.randomUUID();
+        String newToken = "123456789";
+        String nodeMoveString = "{\"jobId\":\"" + jobId + "\",\"jobStatus\":\"SUCCEEDED\",\"instance\":\"127.0.0.1\"}";


Nit: Why do we return instance in response here? OperationalJobResponse seems to return only these 4 fields jobId, jobStatus, operation, reason

sarankk · 2025-12-23T21:30:25Z

server/src/test/java/org/apache/cassandra/sidecar/handlers/NodeMoveHandlerTest.java

+              .expect(ResponsePredicate.SC_OK)
+              .putHeader("content-type", "application/json")
+              .sendBuffer(io.vertx.core.buffer.Buffer.buffer(requestBody), context.succeeding(response -> {
+                  assertThat(response.statusCode()).isEqualTo(OK.code());


Nit: verify "jobStatus":"FAILED", "reason":"Simulated failure"

sarankk · 2025-12-23T21:44:11Z

...egrationTest/org/apache/cassandra/sidecar/routes/CassandraNodeOperationsIntegrationTest.java

+
+        // Validate the operational job status using the OperationalJobHandler
+        String jobId = responseBody.getString("jobId");
+        validateOperationalJobStatus(jobId, "move", OperationalJobStatus.SUCCEEDED);


Clarification question, does storageOps.move call succeed even if the node is in moving status?

sklaha added 6 commits September 29, 2025 21:15

CASSSIDECAR-344: Sidecar endpoint for moving a node to a new token

5def3c4

Merge branch 'trunk' into CASSSIDECAR-344

2031856

Updated node move API to accept newToken in request body

366ed95

Merged from trunk

02e70e1

Fixed merge conflicts

77501a2

Addressed review comments

881126b

sklaha mentioned this pull request Nov 5, 2025

CASSSIDECAR-344: Sidecar endpoint for moving a node to a new token #265

Closed

Merge branch 'trunk' into CASSSIDECAR-344

b768d43

frankgh reviewed Dec 2, 2025

View reviewed changes

sklaha added 4 commits December 15, 2025 06:57

Merge branch 'trunk' into CASSSIDECAR-344

d6622cd

Addressed review comments

0373271

Updated validation for token

8153996

Merge branch 'trunk' into CASSSIDECAR-344

8596a17

frankgh reviewed Dec 22, 2025

View reviewed changes

sklaha requested a review from frankgh December 22, 2025 18:22

Added documentation for MAX_TOKEN_LENGTH

faf1407

frankgh approved these changes Dec 22, 2025

View reviewed changes

sarankk approved these changes Dec 23, 2025

View reviewed changes

	void move(String newToken);
	void move(String newToken) throws IOException;

	this.newToken = newToken;
	this.newToken = Objects.requireNonNull(newToken, "newToken is required");

CASSSIDECAR-344: Sidecar endpoint for moving a node to a new token #275

Are you sure you want to change the base?

CASSSIDECAR-344: Sidecar endpoint for moving a node to a new token #275

Uh oh!

Conversation

sklaha commented Nov 5, 2025

Sidecar endpoint for moving a node to a new token

Uh oh!

frankgh left a comment

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!

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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

frankgh left a comment

Choose a reason for hiding this comment

Uh oh!

sarankk left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants