jsancio commented on code in PR #21122:
URL: https://github.com/apache/kafka/pull/21122#discussion_r2733389492
##########
server-common/src/main/java/org/apache/kafka/server/common/FinalizedFeatures.java:
##########
@@ -19,25 +19,30 @@
import java.util.HashMap;
import java.util.Map;
import java.util.Objects;
+import java.util.Optional;
public record FinalizedFeatures(
- MetadataVersion metadataVersion,
+ Optional<MetadataVersion> metadataVersion,
Map<String, Short> finalizedFeatures,
long finalizedFeaturesEpoch
) {
+ public static final FinalizedFeatures UNKNOWN_FINALIZED_FEATURES =
+ new FinalizedFeatures(Optional.empty(), Map.of(), -1);
+
public static FinalizedFeatures fromKRaftVersion(MetadataVersion version) {
- return new FinalizedFeatures(version, Map.of(), -1);
+ return new FinalizedFeatures(Optional.of(version), Map.of(), -1);
}
public FinalizedFeatures(
- MetadataVersion metadataVersion,
+ Optional<MetadataVersion> metadataVersion,
Map<String, Short> finalizedFeatures,
long finalizedFeaturesEpoch
Review Comment:
It would be nice to limit the possible construction of this type. I see 3
possible values of this object.
1. `FinalizedVersion fromKRaftVersion(MetadataVersion)`
2. `FinalizedVersion unknown()` this is what you call internally
`UNKNOWN_FINALIZED_FEATURES`
3. `FinalizedVersion of(MetadataVersion, Map<String, Short>, long)`
For access the properties of this object you can have:
1. `boolean isMetadataKnown()`
2. `MetadataVersion metadataVersionOrThrow()`
Do support this restrictions, you can't use the `record` syntax and instead
have to `final class` with overridden methods like `equals`, `hashCode` and
`toString`.
What do you think? If you agree, let's add unittest for all of these
possible value types.
##########
server-common/src/main/java/org/apache/kafka/server/common/FinalizedFeatures.java:
##########
@@ -19,25 +19,30 @@
import java.util.HashMap;
import java.util.Map;
import java.util.Objects;
+import java.util.Optional;
public record FinalizedFeatures(
- MetadataVersion metadataVersion,
+ Optional<MetadataVersion> metadataVersion,
Map<String, Short> finalizedFeatures,
long finalizedFeaturesEpoch
) {
+ public static final FinalizedFeatures UNKNOWN_FINALIZED_FEATURES =
+ new FinalizedFeatures(Optional.empty(), Map.of(), -1);
Review Comment:
I suggest making this private and adding a static method that returns this
object.
--
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]
