CASSSIDECAR-344: Sidecar endpoint for moving a node to a new token #265
Conversation
frankgh
left a comment
frankgh
left a comment
There was a problem hiding this comment.
Looks good in general. I added a few comments
| */ | ||
| public NodeMoveRequest(String newToken) | ||
| { | ||
| super(ApiEndpointsV1.NODE_MOVE_ROUTE + "?newToken=" + newToken); |
There was a problem hiding this comment.
should we make the newToken be part of the body payload instead of having it as a query string?
There was a problem hiding this comment.
+1, should be in the body since it is part of the actual resource being updated rather than an optional parameter/filter.
There was a problem hiding this comment.
I accidentally deleted the commits in the PR from my fork, which automatically closed the PR. I’m unable to reopen it. I’ve created a new PR that addresses this comment: #275
| * | ||
| * @param newToken the new token for the node to move to | ||
| */ | ||
| void move(String newToken); |
There was a problem hiding this comment.
I think we need to handle the case where the token does not belong to the node. In Cassandra, it looks like we throw an IOException when token is not valid. Also we should have a test around this.
There was a problem hiding this comment.
I accidentally deleted the commits in the PR from my fork, which automatically closed the PR. I’m unable to reopen it. I’ve created a new PR that addresses this comment: #275
| */ | ||
| public NodeMoveRequest(String newToken) | ||
| { | ||
| super(ApiEndpointsV1.NODE_MOVE_ROUTE + "?newToken=" + newToken); |
There was a problem hiding this comment.
+1, should be in the body since it is part of the actual resource being updated rather than an optional parameter/filter.
| JsonObject streamStats = streamStatsResponse.bodyAsJsonObject(); | ||
| assertThat(streamStats).isNotNull(); | ||
| // The operationMode should be either NORMAL (completed) or MOVING (in progress) | ||
| assertThat(streamStats.getString("operationMode")).isIn("NORMAL", "MOVING"); |
There was a problem hiding this comment.
The concern with this is that we get a false negative when no move happened i.e. node remains in NORMAL state. We would need a more concrete way to validate that a move happened - eg. add data to ensure the move takes longer and evaluate validating the end state via ring or token-ranges endpoint.
| public boolean isRunningOnCassandra() | ||
| { | ||
| String operationMode = storageOperations.operationMode(); | ||
| return OPERATION_MODE_MOVING.equals(operationMode); |
There was a problem hiding this comment.
Question is, do we want to fail-fast when we discover the node is not in a NORMAL state here. It would seem reasonable to do so, but worth checking Cassandra's behavior across versions.
sklaha
force-pushed
the
CASSSIDECAR-344
branch
from
366ed95 to
ef6273c
Compare
3 participants
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: