[GitHub] [kafka] hachikuji commented on a change in pull request #10357: MINOR: KIP-500 Cluster ID is a String

[GitBox <gi...@apache.org>]

hachikuji commented on a change in pull request #10357:
URL: https://github.com/apache/kafka/pull/10357#discussion_r597285632



##########
File path: core/src/main/scala/kafka/server/BrokerMetadataCheckpoint.scala
##########
@@ -106,14 +105,8 @@ object MetaProperties {
     value.getOrElse(throw new RuntimeException(s"Failed to find required property $key."))
   }
 
-  def requireClusterId(properties: RawMetaProperties): Uuid = {
-    val value = require(ClusterIdKey, properties.clusterId)
-    try {
-      Uuid.fromString(value)
-    } catch {
-      case e: Throwable => throw new RuntimeException(s"Failed to parse $ClusterIdKey property " +
-        s"as a UUID: ${e.getMessage}")
-    }
+  def requireClusterId(properties: RawMetaProperties): String = {

Review comment:
       We may as well get rid of this method. We are already using `require(NodeIdKey, properties.nodeId)` above.

##########
File path: core/src/test/scala/kafka/server/BrokerMetadataCheckpointTest.scala
##########
@@ -81,21 +80,21 @@ class BrokerMetadataCheckpointTest {
   }
 
   @Test
-  def testMetaPropertiesDoesNotAllowHexEncodedUUIDs(): Unit = {
+  def testMetaPropertiesAllowsHexEncodedUUIDs(): Unit = {
     val properties = new RawMetaProperties()
     properties.version = 1
     properties.clusterId = "7bc79ca1-9746-42a3-a35a-efb3cde44492"
     properties.nodeId = 1
-    assertThrows(classOf[RuntimeException], () => MetaProperties.parse(properties))
+    MetaProperties.parse(properties)

Review comment:
       Can we assert something about the return value? For example, we could follow the pattern in `testCreateMetadataProperties`.

##########
File path: core/src/test/scala/unit/kafka/server/KafkaRaftServerTest.scala
##########
@@ -32,7 +32,7 @@ class KafkaRaftServerTest {
 
   @Test
   def testSuccessfulLoadMetaProperties(): Unit = {
-    val clusterId = Uuid.randomUuid()
+    val clusterId = Uuid.randomUuid().toString

Review comment:
       Probably doesn't matter too much, but perhaps we could hard-code all of these to some string such as "JgxuGe9URy-E-ceaL04lEw" from the other test. The way it's written after this change looks a little odd.

##########
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 logic above which requires that the clusterId parses as a UUID?




-- 
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:
users@infra.apache.org