Copilot commented on code in PR #21132:
URL: https://github.com/apache/kafka/pull/21132#discussion_r2612145817
##########
core/src/test/scala/unit/kafka/server/StreamsGroupHeartbeatRequestTest.scala:
##########
@@ -111,31 +110,6 @@ class StreamsGroupHeartbeatRequestTest(cluster:
ClusterInstance) extends GroupCo
assertEquals(expectedResponse, streamsGroupHeartbeatResponse)
}
- @ClusterTest(
- serverProperties = Array(
- new ClusterConfigProperty(key =
GroupCoordinatorConfig.GROUP_COORDINATOR_REBALANCE_PROTOCOLS_CONFIG, value =
"classic,consumer,streams"),
- )
- )
- def
testStreamsGroupHeartbeatIsInaccessibleWhenUnstableLatestVersionNotEnabled():
Unit = {
- val topology = new StreamsGroupHeartbeatRequestData.Topology()
- .setEpoch(1)
- .setSubtopologies(List().asJava)
-
- val streamsGroupHeartbeatResponse = streamsGroupHeartbeat(
- groupId = "test-group",
- memberId = "test-member",
- rebalanceTimeoutMs = 1000,
- activeTasks = List.empty,
- standbyTasks = List.empty,
- warmupTasks = List.empty,
- topology = topology,
- expectedError = Errors.NOT_COORDINATOR
- )
-
- val expectedResponse = new
StreamsGroupHeartbeatResponseData().setErrorCode(Errors.NOT_COORDINATOR.code())
- assertEquals(expectedResponse, streamsGroupHeartbeatResponse)
- }
-
@ClusterTest
Review Comment:
The removed test
`testStreamsGroupHeartbeatIsInaccessibleWhenUnstableLatestVersionNotEnabled`
was expecting `Errors.NOT_COORDINATOR` as the error code when the unstable API
version is disabled. However, the other similar tests in this file that check
for API inaccessibility (e.g.,
`testStreamsGroupHeartbeatIsInaccessibleWhenDisabledByFeatureConfig` and
`testStreamsGroupHeartbeatIsInaccessibleWhenDisabledByStaticGroupCoordinatorProtocolConfig`)
expect `Errors.UNSUPPORTED_VERSION`.
The mismatch in expected error codes suggests that either:
1. The removed test had an incorrect expected error code and was not
properly testing the intended behavior, or
2. There was a distinct scenario being tested that is no longer covered
after this removal
Please verify that removing this test doesn't eliminate coverage for a
specific edge case or that the test was indeed incorrect and redundant.
##########
core/src/test/scala/unit/kafka/server/StreamsGroupHeartbeatRequestTest.scala:
##########
@@ -35,7 +35,6 @@ import scala.jdk.CollectionConverters._
serverProperties = Array(
new ClusterConfigProperty(key =
GroupCoordinatorConfig.OFFSETS_TOPIC_PARTITIONS_CONFIG, value = "1"),
new ClusterConfigProperty(key =
GroupCoordinatorConfig.OFFSETS_TOPIC_REPLICATION_FACTOR_CONFIG, value = "1"),
- new ClusterConfigProperty(key = "unstable.api.versions.enable", value =
"true"),
new ClusterConfigProperty(key = "group.coordinator.rebalance.protocols",
value = "classic,consumer,streams"),
new ClusterConfigProperty(key =
"group.streams.initial.rebalance.delay.ms", value = "0")
Review Comment:
The removal of `unstable.api.versions.enable` configuration from this test
class may be premature. This configuration is still actively used in other test
files including:
- `ApiVersionsRequestTest.scala` (lines 32, 42, 72)
- `SaslApiVersionsRequestTest.scala` (lines 45, 85)
- `DescribeFeaturesTest.java` (lines 49, 54)
- `FeatureCommandTest.java` (lines 134, 139)
The configuration constant is also still defined in `ServerConfigs.java`
(line 115: `UNSTABLE_API_VERSIONS_ENABLE_CONFIG`).
Unless there's a specific reason why StreamsGroupHeartbeatRequestTest
doesn't need this configuration while other API version tests do, this removal
should be reconsidered or accompanied by a broader cleanup across all test
files.
```suggestion
new ClusterConfigProperty(key =
"group.streams.initial.rebalance.delay.ms", value = "0"),
new ClusterConfigProperty(key = "unstable.api.versions.enable", value =
"true")
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
