rondagostino commented on a change in pull request #10357:
URL: https://github.com/apache/kafka/pull/10357#discussion_r597324486
##
File path: core/src/main/scala/kafka/tools/StorageTool.scala
##
@@ -198,7 +198,7 @@ object StorageTool extends Logging {
s"does not appear to be a valid UUID: ${e.getMessage}")
}
require(config.nodeId >= 0, s"The node.id must be set to a non-negative
integer.")
-new MetaProperties(effectiveClusterId, config.nodeId)
+new MetaProperties(effectiveClusterId.toString, config.nodeId)
Review comment:
> Should we also remove the [StorageTool] logic above which requires
that the clusterId parses as a UUID
Basically, this is asking if `StorageTool` should support accepting
non-UUIDs via its `--cluster-id` argument. One purpose of the tool is to
minimize the chance that a broker could use data from the wrong volume (i.e.
data from another cluster). Generating a random UUID via the `--random-uuid`
parameter encourages using a globally unique value for every cluster and is
consistent with the behavior today with ZooKeeper, whereas allowing a non-UUID
here would increase the chance that someone could reuse a Cluster ID value
across clusters and short-circuit the risk mitigation that this tool provides.
Merging this PR now avoids an uncomfortable "rename" in
`BrokerRegistrationRequest` (or a breaking change) after 2.8 is released;
letting `StorageTool` accept non-UUIDs can be done at any time with no impact.
Maybe we keep this PR about removing the constraint, which also simply cannot
exist -- otherwise we can't upgrade clusters that don't use a UUID -- and leave
this question about the `--cluster-id` parameter for a broader discussion
later?
--
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
