hachikuji commented on a change in pull request #11667:
URL: https://github.com/apache/kafka/pull/11667#discussion_r802153736
##########
File path:
core/src/test/scala/unit/kafka/server/AbstractApiVersionsRequestTest.scala
##########
@@ -34,18 +36,25 @@ import scala.jdk.CollectionConverters._
abstract class AbstractApiVersionsRequestTest(cluster: ClusterInstance) {
def sendApiVersionsRequest(request: ApiVersionsRequest, listenerName:
ListenerName): ApiVersionsResponse = {
- IntegrationTestUtils.connectAndReceive[ApiVersionsResponse](request,
cluster.brokerSocketServers().asScala.head, listenerName)
+ val socket = if (listenerName == controlPlaneListenerName) {
+ cluster.controllerSocketServers().asScala.head
+ } else {
+ cluster.brokerSocketServers().asScala.head
+ }
+ IntegrationTestUtils.connectAndReceive[ApiVersionsResponse](request,
socket, listenerName)
}
def controlPlaneListenerName = new ListenerName("CONTROLLER")
Review comment:
Not from this patch, but I found the overloading here a bit confusing.
The control plane listener is only relevant to zk clusters, but here we're
using the CONTROLLER name so that we can also use it to designate the
controller for kraft clusters (as in `sendApiVersionsRequest` above). I wonder
if it would be clearer to have two separate methods:
```scala
def controlPlaneListener: Option[ListenerName] = if (cluster.isKRaftTest)
None else Some(new ListenerName("CONTROL_PLANE"))
def controllerListener: Option[ListenerName] = if (cluster.isKRaftTest)
Some(new ListenerName("CONTROLLER") else None
```
Then maybe we could have separate test cases:
```scala
@ClusterTest(clusterType = ClusterType.ZK)
def testApiVersionsRequestThroughControlPlaneListener(): Unit = ???
@ClusterTest(clusterType = ClusterType.RAFT)
def testApiVersionsRequestThroughControllerListener(): Unit = ???
```
What do you think?
##########
File path: clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java
##########
@@ -98,7 +98,7 @@
END_QUORUM_EPOCH(ApiMessageType.END_QUORUM_EPOCH, true,
RecordBatch.MAGIC_VALUE_V0, false),
DESCRIBE_QUORUM(ApiMessageType.DESCRIBE_QUORUM, true,
RecordBatch.MAGIC_VALUE_V0, true),
ALTER_ISR(ApiMessageType.ALTER_ISR, true),
- UPDATE_FEATURES(ApiMessageType.UPDATE_FEATURES, false, true),
+ UPDATE_FEATURES(ApiMessageType.UPDATE_FEATURES),
Review comment:
Fair enough. I think we'll reenable forwarding in
https://github.com/apache/kafka/pull/11677, but for now, this is fine.
##########
File path:
core/src/test/scala/unit/kafka/server/AbstractApiVersionsRequestTest.scala
##########
@@ -59,15 +68,40 @@ abstract class AbstractApiVersionsRequestTest(cluster:
ClusterInstance) {
} finally socket.close()
}
- def validateApiVersionsResponse(apiVersionsResponse: ApiVersionsResponse):
Unit = {
- val expectedApis = ApiKeys.zkBrokerApis()
+ def validateApiVersionsResponse(apiVersionsResponse: ApiVersionsResponse,
controllerApi: Boolean = false): Unit = {
Review comment:
Instead of passing through `controllerApi` as a flag, maybe we could
pass the listener?
--
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
