From dce48a70a1ef908fef85e046e8babdb65eb60667 Mon Sep 17 00:00:00 2001 From: Ashish Singh Date: Wed, 27 Apr 2016 11:48:40 -0700 Subject: [PATCH 1/5] KAFKA-3621: Add tests for ApiVersionRequest/Response --- .../common/requests/ApiVersionsResponse.java | 14 +++++++ .../kafka/server/ApiVersionsRequestTest.scala | 41 +++++++++++++++++++ .../unit/kafka/server/BaseRequestTest.scala | 8 ++-- 3 files changed, 60 insertions(+), 3 deletions(-) create mode 100644 core/src/test/scala/unit/kafka/server/ApiVersionsRequestTest.scala diff --git a/clients/src/main/java/org/apache/kafka/common/requests/ApiVersionsResponse.java b/clients/src/main/java/org/apache/kafka/common/requests/ApiVersionsResponse.java index 36881a3e090cb..d3863b49b6a17 100644 --- a/clients/src/main/java/org/apache/kafka/common/requests/ApiVersionsResponse.java +++ b/clients/src/main/java/org/apache/kafka/common/requests/ApiVersionsResponse.java @@ -54,6 +54,20 @@ public ApiVersion(short apiKey, short minVersion, short maxVersion) { this.minVersion = minVersion; this.maxVersion = maxVersion; } + + @Override + public boolean equals(Object obj) { + if (obj == this) + return true; + + if (!(obj instanceof ApiVersion)) + return false; + + ApiVersion other = (ApiVersion) obj; + return other.apiKey == this.apiKey && + other.minVersion == this.minVersion && + other.maxVersion == this.maxVersion; + } } public ApiVersionsResponse(short errorCode, List apiVersions) { diff --git a/core/src/test/scala/unit/kafka/server/ApiVersionsRequestTest.scala b/core/src/test/scala/unit/kafka/server/ApiVersionsRequestTest.scala new file mode 100644 index 0000000000000..f0422d1e57b51 --- /dev/null +++ b/core/src/test/scala/unit/kafka/server/ApiVersionsRequestTest.scala @@ -0,0 +1,41 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package kafka.server + +import org.apache.kafka.common.protocol.ApiKeys +import org.apache.kafka.common.requests.{ApiVersionsRequest, ApiVersionsResponse} +import org.junit.Assert._ +import org.junit.Test + +class ApiVersionsRequestTest extends BaseRequestTest { + + override def numBrokers(): Int = 1 + + @Test + def testApiVersionsRequest() { + val apiVersionsResponse = sendApiVersionsRequest(new ApiVersionsRequest, 0) + + assertEquals("API keys in ApiVersionsResponse must match API keys supported by broker.", ApiKeys.values.length, apiVersionsResponse.apiVersions.size) + assertTrue("API versions in ApiVersionsResponse must be supported by broker.", KafkaApis.apiVersionsResponse.apiVersions().containsAll(apiVersionsResponse.apiVersions())) + } + + private def sendApiVersionsRequest(request: ApiVersionsRequest, version: Short): ApiVersionsResponse = { + val response = send(request, ApiKeys.API_VERSIONS, version) + ApiVersionsResponse.parse(response) + } +} \ No newline at end of file diff --git a/core/src/test/scala/unit/kafka/server/BaseRequestTest.scala b/core/src/test/scala/unit/kafka/server/BaseRequestTest.scala index 3d05c1d34629e..010b3c3575fa6 100644 --- a/core/src/test/scala/unit/kafka/server/BaseRequestTest.scala +++ b/core/src/test/scala/unit/kafka/server/BaseRequestTest.scala @@ -30,11 +30,13 @@ import org.apache.kafka.common.requests.{AbstractRequest, RequestHeader, Respons import org.junit.Before abstract class BaseRequestTest extends KafkaServerTestHarness { - val numBrokers = 3 private var correlationId = 0 - // Override properties by mutating the passed Properties object - def propertyOverrides(properties: Properties): Unit + // If required, set number of brokers + protected def numBrokers(): Int = 3 + + // If required, override properties by mutating the passed Properties object + protected def propertyOverrides(properties: Properties): Unit = {} def generateConfigs() = { val props = TestUtils.createBrokerConfigs(numBrokers, zkConnect, enableControlledShutdown = false) From f3e61b7cd546556f9d3b5821ac0fea482646c133 Mon Sep 17 00:00:00 2001 From: Ashish Singh Date: Wed, 27 Apr 2016 12:46:34 -0700 Subject: [PATCH 2/5] Use procedure notation in BaseRequestTest.propertyOverrides --- core/src/test/scala/unit/kafka/server/BaseRequestTest.scala | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/src/test/scala/unit/kafka/server/BaseRequestTest.scala b/core/src/test/scala/unit/kafka/server/BaseRequestTest.scala index 010b3c3575fa6..d92ccea81e19d 100644 --- a/core/src/test/scala/unit/kafka/server/BaseRequestTest.scala +++ b/core/src/test/scala/unit/kafka/server/BaseRequestTest.scala @@ -33,10 +33,10 @@ abstract class BaseRequestTest extends KafkaServerTestHarness { private var correlationId = 0 // If required, set number of brokers - protected def numBrokers(): Int = 3 + protected def numBrokers: Int = 3 // If required, override properties by mutating the passed Properties object - protected def propertyOverrides(properties: Properties): Unit = {} + protected def propertyOverrides(properties: Properties) {} def generateConfigs() = { val props = TestUtils.createBrokerConfigs(numBrokers, zkConnect, enableControlledShutdown = false) From 757a39e5901d079bf39cbbb620d06a1c1ec8e4b1 Mon Sep 17 00:00:00 2001 From: Ashish Singh Date: Wed, 27 Apr 2016 16:28:57 -0700 Subject: [PATCH 3/5] Remove equals from ApiVersion --- .../kafka/common/requests/ApiVersionsResponse.java | 14 -------------- .../unit/kafka/server/ApiVersionsRequestTest.scala | 11 ++++++++++- 2 files changed, 10 insertions(+), 15 deletions(-) diff --git a/clients/src/main/java/org/apache/kafka/common/requests/ApiVersionsResponse.java b/clients/src/main/java/org/apache/kafka/common/requests/ApiVersionsResponse.java index d3863b49b6a17..36881a3e090cb 100644 --- a/clients/src/main/java/org/apache/kafka/common/requests/ApiVersionsResponse.java +++ b/clients/src/main/java/org/apache/kafka/common/requests/ApiVersionsResponse.java @@ -54,20 +54,6 @@ public ApiVersion(short apiKey, short minVersion, short maxVersion) { this.minVersion = minVersion; this.maxVersion = maxVersion; } - - @Override - public boolean equals(Object obj) { - if (obj == this) - return true; - - if (!(obj instanceof ApiVersion)) - return false; - - ApiVersion other = (ApiVersion) obj; - return other.apiKey == this.apiKey && - other.minVersion == this.minVersion && - other.maxVersion == this.maxVersion; - } } public ApiVersionsResponse(short errorCode, List apiVersions) { diff --git a/core/src/test/scala/unit/kafka/server/ApiVersionsRequestTest.scala b/core/src/test/scala/unit/kafka/server/ApiVersionsRequestTest.scala index f0422d1e57b51..b1af8548fafb7 100644 --- a/core/src/test/scala/unit/kafka/server/ApiVersionsRequestTest.scala +++ b/core/src/test/scala/unit/kafka/server/ApiVersionsRequestTest.scala @@ -18,10 +18,13 @@ package kafka.server import org.apache.kafka.common.protocol.ApiKeys +import org.apache.kafka.common.requests.ApiVersionsResponse.ApiVersion import org.apache.kafka.common.requests.{ApiVersionsRequest, ApiVersionsResponse} import org.junit.Assert._ import org.junit.Test +import scala.collection.JavaConversions._ + class ApiVersionsRequestTest extends BaseRequestTest { override def numBrokers(): Int = 1 @@ -31,7 +34,13 @@ class ApiVersionsRequestTest extends BaseRequestTest { val apiVersionsResponse = sendApiVersionsRequest(new ApiVersionsRequest, 0) assertEquals("API keys in ApiVersionsResponse must match API keys supported by broker.", ApiKeys.values.length, apiVersionsResponse.apiVersions.size) - assertTrue("API versions in ApiVersionsResponse must be supported by broker.", KafkaApis.apiVersionsResponse.apiVersions().containsAll(apiVersionsResponse.apiVersions())) + for (expectedApiVersion: ApiVersion <- KafkaApis.apiVersionsResponse.apiVersions()) { + val actualApiVersion = apiVersionsResponse.apiVersion(expectedApiVersion.apiKey) + assertNotNull(s"API key ${actualApiVersion.apiKey} is supported by broker, but not received in ApiVersionsResponse.", actualApiVersion) + assertEquals(s"API key ${actualApiVersion.apiKey} must be supported by broker.", expectedApiVersion.apiKey, actualApiVersion.apiKey) + assertEquals(s"API key ${actualApiVersion.apiKey} must have ${expectedApiVersion.minVersion} as minimum supported version.", expectedApiVersion.minVersion, actualApiVersion.minVersion) + assertEquals(s"API key ${actualApiVersion.apiKey} must have ${expectedApiVersion.maxVersion} as maximum supported version.", expectedApiVersion.maxVersion, actualApiVersion.maxVersion) + } } private def sendApiVersionsRequest(request: ApiVersionsRequest, version: Short): ApiVersionsResponse = { From 8999512de1db0976c92e550d0968c730a23cb728 Mon Sep 17 00:00:00 2001 From: Ashish Singh Date: Wed, 27 Apr 2016 16:48:12 -0700 Subject: [PATCH 4/5] Make assert comments less verbose --- .../scala/unit/kafka/server/ApiVersionsRequestTest.scala | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/core/src/test/scala/unit/kafka/server/ApiVersionsRequestTest.scala b/core/src/test/scala/unit/kafka/server/ApiVersionsRequestTest.scala index b1af8548fafb7..50acb03ed52e7 100644 --- a/core/src/test/scala/unit/kafka/server/ApiVersionsRequestTest.scala +++ b/core/src/test/scala/unit/kafka/server/ApiVersionsRequestTest.scala @@ -27,7 +27,7 @@ import scala.collection.JavaConversions._ class ApiVersionsRequestTest extends BaseRequestTest { - override def numBrokers(): Int = 1 + override def numBrokers: Int = 1 @Test def testApiVersionsRequest() { @@ -37,9 +37,9 @@ class ApiVersionsRequestTest extends BaseRequestTest { for (expectedApiVersion: ApiVersion <- KafkaApis.apiVersionsResponse.apiVersions()) { val actualApiVersion = apiVersionsResponse.apiVersion(expectedApiVersion.apiKey) assertNotNull(s"API key ${actualApiVersion.apiKey} is supported by broker, but not received in ApiVersionsResponse.", actualApiVersion) - assertEquals(s"API key ${actualApiVersion.apiKey} must be supported by broker.", expectedApiVersion.apiKey, actualApiVersion.apiKey) - assertEquals(s"API key ${actualApiVersion.apiKey} must have ${expectedApiVersion.minVersion} as minimum supported version.", expectedApiVersion.minVersion, actualApiVersion.minVersion) - assertEquals(s"API key ${actualApiVersion.apiKey} must have ${expectedApiVersion.maxVersion} as maximum supported version.", expectedApiVersion.maxVersion, actualApiVersion.maxVersion) + assertEquals("API key must be supported by the broker.", expectedApiVersion.apiKey, actualApiVersion.apiKey) + assertEquals(s"Received unexpected min version for API key ${actualApiVersion.apiKey}.", expectedApiVersion.minVersion, actualApiVersion.minVersion) + assertEquals(s"Received unexpected max version for API key ${actualApiVersion.apiKey}.", expectedApiVersion.maxVersion, actualApiVersion.maxVersion) } } From 92dcde4951370594475cc5c9026ef20683351aa6 Mon Sep 17 00:00:00 2001 From: Ashish Singh Date: Wed, 27 Apr 2016 17:01:48 -0700 Subject: [PATCH 5/5] Another lying around () in scala code. --- .../test/scala/unit/kafka/server/ApiVersionsRequestTest.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/test/scala/unit/kafka/server/ApiVersionsRequestTest.scala b/core/src/test/scala/unit/kafka/server/ApiVersionsRequestTest.scala index 50acb03ed52e7..ed599300c3f04 100644 --- a/core/src/test/scala/unit/kafka/server/ApiVersionsRequestTest.scala +++ b/core/src/test/scala/unit/kafka/server/ApiVersionsRequestTest.scala @@ -34,7 +34,7 @@ class ApiVersionsRequestTest extends BaseRequestTest { val apiVersionsResponse = sendApiVersionsRequest(new ApiVersionsRequest, 0) assertEquals("API keys in ApiVersionsResponse must match API keys supported by broker.", ApiKeys.values.length, apiVersionsResponse.apiVersions.size) - for (expectedApiVersion: ApiVersion <- KafkaApis.apiVersionsResponse.apiVersions()) { + for (expectedApiVersion: ApiVersion <- KafkaApis.apiVersionsResponse.apiVersions) { val actualApiVersion = apiVersionsResponse.apiVersion(expectedApiVersion.apiKey) assertNotNull(s"API key ${actualApiVersion.apiKey} is supported by broker, but not received in ApiVersionsResponse.", actualApiVersion) assertEquals("API key must be supported by the broker.", expectedApiVersion.apiKey, actualApiVersion.apiKey)