nvharikrishna commented on code in PR #4614:
URL: https://github.com/apache/cassandra/pull/4614#discussion_r2838256297
##########
src/java/org/apache/cassandra/tcm/transformations/Register.java:
##########
@@ -74,6 +74,22 @@ public Kind kind()
@Override
public Result execute(ClusterMetadata prev)
{
+ // Ensure the joining node can read existing cluster metadata.
+ // Skip check for empty directory (first node in a new cluster).
+ if (!prev.directory.isEmpty())
+ {
+ Version clusterVersion = prev.directory.commonSerializationVersion;
+ Version newNodeVersion = version.serializationVersion();
+ if (newNodeVersion.isBefore(clusterVersion))
+ {
+ return new Rejected(INVALID,
+ String.format("Cannot register node: this
node's metadata serialization version %s " +
Review Comment:
> The recovery strategy here is to upgrade the joining node to a cassandra
version that supports the required metadata serialization version
It could be stated explicitly so that operators who are not familiar with
TCM would be able to understand what to do next. At multiple places in TCM, the
log messages suggest what to do next (ref:
[1](https://github.com/apache/cassandra/blob/trunk/src/java/org/apache/cassandra/tcm/sequences/BootstrapAndReplace.java#L436)
[2](https://github.com/apache/cassandra/blob/trunk/src/java/org/apache/cassandra/tcm/transformations/PrepareLeave.java#L155)).
##########
src/java/org/apache/cassandra/tcm/transformations/Startup.java:
##########
@@ -68,6 +68,19 @@ public Kind kind()
@Override
public Result execute(ClusterMetadata prev)
{
+ // Prevent downgrade to a version that cannot read cluster metadata.
+ // This protects against restarting a node with an older binary.
+ Version clusterVersion = prev.directory.commonSerializationVersion;
+ Version newNodeVersion = nodeVersion.serializationVersion();
+ if (newNodeVersion.isBefore(clusterVersion))
Review Comment:
Apologies for the confusion! When I mentioned de-duplicating the code, I
should have added more details. The first thing that came to mind after seeing
the `nodeVersion.serializationVersion()` usage was why it didn’t use
`NodeVersion.Current.serializationVersion`. I understand the reason. I also
noticed that the same check has been added in another place. I thought having a
common check is less prone to mistakes in the future. I was thinking more like
`PrepareLeave#validateReplicationForDecommission`[[ref](https://github.com/apache/cassandra/blob/trunk/src/java/org/apache/cassandra/tcm/transformations/PrepareLeave.java#L139)]
and how it’s used in
`PrepareLeave#execute`[[ref](https://github.com/apache/cassandra/blob/trunk/src/java/org/apache/cassandra/tcm/transformations/PrepareLeave.java#L97)].
The validate method checks the conditions and logs an issue message with a
solution. The transformation's execute method executes this validation
(conditionally) and rejects the transfo
rmation if necessary.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
