ijuma commented on a change in pull request #9746:
URL: https://github.com/apache/kafka/pull/9746#discussion_r559853597
##########
File path: clients/src/main/java/org/apache/kafka/clients/NodeApiVersions.java
##########
@@ -39,18 +38,18 @@
public class NodeApiVersions {
// A map of the usable versions of each API, keyed by the ApiKeys instance
- private final Map<ApiKeys, ApiVersion> supportedVersions = new
EnumMap<>(ApiKeys.class);
+ private final Map<ApiKeys, ApiVersionsResponseData.ApiVersion>
supportedVersions = new EnumMap<>(ApiKeys.class);
Review comment:
Can we import `ApiVersion` directly? I don't think
`ApiVersionsResponseData` adds any value here and other similar places.
##########
File path: clients/src/main/java/org/apache/kafka/clients/NodeApiVersions.java
##########
@@ -60,18 +59,21 @@ public static NodeApiVersions create() {
* value.
* @return A new NodeApiVersions object.
*/
- public static NodeApiVersions create(Collection<ApiVersion> overrides) {
- List<ApiVersion> apiVersions = new LinkedList<>(overrides);
+ public static NodeApiVersions
create(Collection<ApiVersionsResponseData.ApiVersion> overrides) {
+ List<ApiVersionsResponseData.ApiVersion> apiVersions = new
LinkedList<>(overrides);
for (ApiKeys apiKey : ApiKeys.enabledApis()) {
boolean exists = false;
- for (ApiVersion apiVersion : apiVersions) {
- if (apiVersion.apiKey == apiKey.id) {
+ for (ApiVersionsResponseData.ApiVersion apiVersion : apiVersions) {
+ if (apiVersion.apiKey() == apiKey.id) {
exists = true;
break;
}
}
if (!exists) {
- apiVersions.add(new ApiVersion(apiKey));
+ apiVersions.add(new ApiVersionsResponseData.ApiVersion()
+ .setApiKey(apiKey.id)
+ .setMinVersion(apiKey.oldestVersion())
+ .setMaxVersion(apiKey.latestVersion()));
Review comment:
It would be good to have a helper method somewhere that converts
`ApiKeys` to `ApiVersion`.
##########
File path:
clients/src/test/java/org/apache/kafka/clients/NodeApiVersionsTest.java
##########
@@ -56,12 +54,18 @@ public void testUnknownApiVersionsToString() {
@Test
public void testVersionsToString() {
- List<ApiVersion> versionList = new ArrayList<>();
+ List<ApiVersionsResponseData.ApiVersion> versionList = new
ArrayList<>();
for (ApiKeys apiKey : ApiKeys.values()) {
if (apiKey == ApiKeys.DELETE_TOPICS) {
- versionList.add(new ApiVersion(apiKey.id, (short) 10000,
(short) 10001));
+ versionList.add(new ApiVersionsResponseData.ApiVersion()
+ .setApiKey(apiKey.id)
+ .setMinVersion((short) 10000)
+ .setMaxVersion((short) 10001));
} else {
- versionList.add(new ApiVersion(apiKey));
+ versionList.add(new ApiVersionsResponseData.ApiVersion()
+ .setApiKey(apiKey.id)
+ .setMinVersion(apiKey.oldestVersion())
+ .setMaxVersion(apiKey.latestVersion()));
Review comment:
We can use the helper that converts from ApiKey to ApiVersion here too.
##########
File path:
clients/src/main/java/org/apache/kafka/common/requests/ApiVersionsResponse.java
##########
@@ -220,4 +217,16 @@ private static FinalizedFeatureKeyCollection
createFinalizedFeatureKeys(
return converted;
}
+
+ public static Optional<ApiVersionsResponseData.ApiVersion>
intersect(ApiVersionsResponseData.ApiVersion thisVersion,
+
ApiVersionsResponseData.ApiVersion other) {
+ if (other == null) return Optional.empty();
Review comment:
It's a bit weird that we only check if `other` is null. Do we need this
check at all?
----------------------------------------------------------------
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:
us...@infra.apache.org
