MINOR: Remove unstable.api.version.enable config and corresponding test from StreamsGroupHeartbeatRequestTest #21132
Conversation
github-actions
bot
added
triage
There was a problem hiding this comment.
Pull request overview
This PR removes the unstable.api.versions.enable configuration setting from StreamsGroupHeartbeatRequestTest and eliminates the associated test testStreamsGroupHeartbeatIsInaccessibleWhenUnstableLatestVersionNotEnabled. The changes aim to simplify the test setup by removing a configuration deemed unnecessary for this specific test class.
Key Changes
- Removed
unstable.api.versions.enableconfiguration from the@ClusterTestDefaultsserverProperties annotation - Removed test method
testStreamsGroupHeartbeatIsInaccessibleWhenUnstableLatestVersionNotEnabledthat verified behavior when the unstable API version config is disabled
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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") |
There was a problem hiding this 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.
| new ClusterConfigProperty(key = "group.streams.initial.rebalance.delay.ms", value = "0") | |
| new ClusterConfigProperty(key = "group.streams.initial.rebalance.delay.ms", value = "0"), | |
| new ClusterConfigProperty(key = "unstable.api.versions.enable", value = "true") |
| assertEquals(expectedResponse, streamsGroupHeartbeatResponse) | ||
| } | ||
|
|
||
| @ClusterTest |
There was a problem hiding this 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:
- The removed test had an incorrect expected error code and was not properly testing the intended behavior, or
- 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.
chia7712
left a comment
chia7712
left a comment
There was a problem hiding this comment.
@lucliu1108 thanks for this clean-up. I have one minor question left
| standbyTasks = List.empty, | ||
| warmupTasks = List.empty, | ||
| topology = topology, | ||
| expectedError = Errors.NOT_COORDINATOR |
There was a problem hiding this comment.
This error appears to be caused by the non-existence of the offset topic. Perhaps we can keep the test, but rename it to reflect this specific scenario
There was a problem hiding this comment.
We could do that as well I guess
3 participants
Summary
unstable.api.version.enableconfig since it's not required.testStreamsGroupHeartbeatIsInaccessibleWhenUnstableLatestVersionNotEnabledtest
Reviewers: @lucasbru