ahuang98 commented on code in PR #12050:
URL: https://github.com/apache/kafka/pull/12050#discussion_r869544110
##########
clients/src/main/java/org/apache/kafka/clients/NodeApiVersions.java:
##########
@@ -91,6 +97,23 @@ public static NodeApiVersions create(short apiKey, short
minVersion, short maxVe
.setMaxVersion(maxVersion)));
}
+ public NodeApiVersions(ApiVersionCollection nodeApiVersions,
SupportedFeatureKeyCollection nodeSupportedFeatures) {
+ for (ApiVersion nodeApiVersion : nodeApiVersions) {
Review Comment:
Can we call `NodeApiVersions(ApiVersionCollection nodeApiVersions)` here to
reduce some redundant code?
##########
server-common/src/main/java/org/apache/kafka/server/common/MetadataVersion.java:
##########
@@ -273,6 +301,40 @@ public static MetadataVersion latest() {
return VALUES[VALUES.length - 1];
}
+ public Optional<MetadataVersion> previous() {
+ int idx = this.ordinal();
+ if (idx > 2) {
+ return Optional.of(MetadataVersion.values()[idx - 1]);
+ } else {
+ return Optional.empty();
+ }
+ }
+
+ public static boolean checkIfMetadataChanged(MetadataVersion
sourceVersion, MetadataVersion targetVersion) {
Review Comment:
Just clarifying, this is to check if the metadata has changed in an
incompatible way between the sourceVersion and targetVersion?
##########
server-common/src/main/java/org/apache/kafka/server/common/MetadataVersion.java:
##########
@@ -273,6 +301,40 @@ public static MetadataVersion latest() {
return VALUES[VALUES.length - 1];
}
+ public Optional<MetadataVersion> previous() {
+ int idx = this.ordinal();
+ if (idx > 2) {
+ return Optional.of(MetadataVersion.values()[idx - 1]);
Review Comment:
```suggestion
return Optional.of(VALUES[idx]);
```
The indexing is a bit unintuitive, I wonder if you might just want to store
all of `values()` (including `UNINITIALIZED`) in `VALUES` just to make this
more clear?
##########
server-common/src/main/java/org/apache/kafka/server/common/MetadataVersion.java:
##########
@@ -273,6 +301,40 @@ public static MetadataVersion latest() {
return VALUES[VALUES.length - 1];
}
+ public Optional<MetadataVersion> previous() {
+ int idx = this.ordinal();
+ if (idx > 2) {
+ return Optional.of(MetadataVersion.values()[idx - 1]);
+ } else {
+ return Optional.empty();
+ }
+ }
+
+ public static boolean checkIfMetadataChanged(MetadataVersion
sourceVersion, MetadataVersion targetVersion) {
Review Comment:
Let's add a test for this
##########
server-common/src/main/java/org/apache/kafka/server/common/MetadataVersion.java:
##########
@@ -273,6 +301,40 @@ public static MetadataVersion latest() {
return VALUES[VALUES.length - 1];
}
+ public Optional<MetadataVersion> previous() {
+ int idx = this.ordinal();
Review Comment:
The indexing for `ordinal()` starts at `0` right? With
`MetadataVersion.UNINITIALIZED` at index 0 (which I'm assuming we don't want to
count as a valid previous version), should the conditional be `if (idx > 1)`?
--
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
