soarez commented on code in PR #14290:
URL: https://github.com/apache/kafka/pull/14290#discussion_r1377451178
##########
server-common/src/main/java/org/apache/kafka/common/DirectoryId.java:
##########
@@ -16,55 +16,166 @@
*/
package org.apache.kafka.common;
+import java.util.ArrayList;
+import java.util.Arrays;
import java.util.Collections;
+import java.util.HashMap;
import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
import java.util.Set;
-public class DirectoryId {
+public class DirectoryId extends Uuid {
Review Comment:
I don't disagree with the general subclassing rule of thumb, but I can't see
why it's bad on this case.
On the contrary, having a specific separate type helps enforce that the IDs
are created from the `DirectoryId` class, not from the `UUID` class. This is
useful because the names and reserved values are different. e.g. without a
separate type, there are no compiler errors/warnings if:
* `Uuid.ZERO_UUID` is used to directly replace `DirectoryId.UNASSINGNED`
creating confusion with the nomenclature
* `Uuid.randomUuid()` is used to directly replace `DirectoryId.random()`,
silently ignoring the possibility that one of the reserved values may be
incorrectly used.
I don't think this is a huge deal either way though, so I you still feel
strongly about this I'm ready – albeit somewhat reluctantly I'll admit – to
change this and follow your suggestion.
Alternatively we could also keep a separate type, but use composition
instead of inheritance. That will require a bit more boilerplate, and AFAICT
bring little extra value, but we'd still get the benefits of having a separate
type.
##########
server-common/src/main/java/org/apache/kafka/common/DirectoryId.java:
##########
@@ -16,55 +16,166 @@
*/
package org.apache.kafka.common;
+import java.util.ArrayList;
+import java.util.Arrays;
import java.util.Collections;
+import java.util.HashMap;
import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
import java.util.Set;
-public class DirectoryId {
+public class DirectoryId extends Uuid {
Review Comment:
I don't disagree with the general subclassing rule of thumb, but I can't see
why it's bad on this case.
On the contrary, having a specific separate type helps enforce that the IDs
are created from the `DirectoryId` class, not from the `UUID` class. This is
useful because the names and reserved values are different. e.g. without a
separate type, there are no compiler errors/warnings if:
* `Uuid.ZERO_UUID` is used to directly replace `DirectoryId.UNASSINGNED`
creating confusion with the nomenclature
* `Uuid.randomUuid()` is used to directly replace `DirectoryId.random()`,
silently ignoring the possibility that one of the reserved values may be
incorrectly used.
I don't think this is a huge deal either way though, so I you still feel
strongly about this I'm ready – albeit somewhat reluctantly I'll admit – to
change this and follow your suggestion.
Alternatively we could also keep a separate type, but use composition
instead of inheritance. That will require a bit more boilerplate, and AFAICT
bring little extra value, but we'd still get the benefits of having a separate
type.
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
