You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2022/05/10 21:23:27 UTC
[GitHub] [kafka] ahuang98 commented on a diff in pull request #12050: KAFKA-13830 MetadataVersion integration for KRaft controller
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org