m1a2st commented on code in PR #18845:
URL: https://github.com/apache/kafka/pull/18845#discussion_r1952564479
##########
metadata/src/main/java/org/apache/kafka/controller/ActivationRecordsGenerator.java:
##########
@@ -153,7 +153,7 @@ static ControllerResult<Void> recordsForNonEmptyLog(
* </p>
* If the log is empty, write the bootstrap records. If the log is not
empty, do some validation and
* possibly write some records to put the log into a valid state. For
bootstrap records, if KIP-868
- * metadata transactions are supported, ues them. Otherwise, write the
bootstrap records as an
+ * metadata transactions are supported, use them. Otherwise, write the
bootstrap records as an
* atomic batch. The single atomic batch can be problematic if the
bootstrap records are too large
* (e.g., lots of SCRAM credentials). If ZK migrations are enabled, the
activation records will
* include a ZkMigrationState record regardless of whether the log was
empty or not.
Review Comment:
Please also cleanup this outdate ZK migrations comment
##########
core/src/test/scala/unit/kafka/server/ApiVersionsResponseIntegrationTest.scala:
##########
@@ -38,28 +37,18 @@ class ApiVersionsResponseIntegrationTest extends
BaseRequestTest {
connectAndReceive[ApiVersionsResponse](request)
}
- @ParameterizedTest
- @ValueSource(strings = Array("kraft"))
- def testSendV3ApiVersionsRequest(quorum: String): Unit = {
+ @Test
+ def testSendV3ApiVersionsRequest(): Unit = {
val response = sendApiVersionsRequest(3)
- if (quorum.equals("kraft")) {
- assertFeatureHasMinVersion("metadata.version",
response.data().supportedFeatures(), 1)
- assertFeatureMissing("kraft.version",
response.data().supportedFeatures())
- } else {
- assertEquals(0, response.data().supportedFeatures().size())
- }
+ assertFeatureHasMinVersion("metadata.version",
response.data().supportedFeatures(), 7)
+ assertFeatureMissing("kraft.version", response.data().supportedFeatures())
}
- @ParameterizedTest
- @ValueSource(strings = Array("kraft"))
- def testSendV4ApiVersionsRequest(quorum: String): Unit = {
+ @Test
+ def testSendV4ApiVersionsRequest(): Unit = {
val response = sendApiVersionsRequest(4)
- if (quorum.equals("kraft")) {
- assertFeatureHasMinVersion("metadata.version",
response.data().supportedFeatures(), 1)
- assertFeatureHasMinVersion("kraft.version",
response.data().supportedFeatures(), 0)
- } else {
- assertEquals(0, response.data().supportedFeatures().size())
- }
+ assertFeatureHasMinVersion("metadata.version",
response.data().supportedFeatures(), 7)
Review Comment:
ditto `MINIMUM_VERSION.featureLevel()` instead of 7 ?
##########
core/src/main/scala/kafka/server/AlterPartitionManager.scala:
##########
@@ -224,53 +215,35 @@ class DefaultAlterPartitionManager(
private def buildRequest(
inflightAlterPartitionItems: Seq[AlterPartitionItem],
brokerEpoch: Long
Review Comment:
Please also update the return value document.
##########
core/src/test/scala/unit/kafka/server/AlterPartitionManagerTest.scala:
##########
@@ -552,24 +535,10 @@ class AlterPartitionManagerTest {
}
object AlterPartitionManagerTest {
- def provideMetadataVersions(): JStream[MetadataVersion] = {
+ def provideLeaderRecoveryState(): JStream[Arguments] = {
Review Comment:
we can use `@EnumSource(classOf[LeaderRecoveryState])`, thus this method can
remove
##########
core/src/test/scala/unit/kafka/server/ApiVersionsResponseIntegrationTest.scala:
##########
@@ -38,28 +37,18 @@ class ApiVersionsResponseIntegrationTest extends
BaseRequestTest {
connectAndReceive[ApiVersionsResponse](request)
}
- @ParameterizedTest
- @ValueSource(strings = Array("kraft"))
- def testSendV3ApiVersionsRequest(quorum: String): Unit = {
+ @Test
+ def testSendV3ApiVersionsRequest(): Unit = {
val response = sendApiVersionsRequest(3)
- if (quorum.equals("kraft")) {
- assertFeatureHasMinVersion("metadata.version",
response.data().supportedFeatures(), 1)
- assertFeatureMissing("kraft.version",
response.data().supportedFeatures())
- } else {
- assertEquals(0, response.data().supportedFeatures().size())
- }
+ assertFeatureHasMinVersion("metadata.version",
response.data().supportedFeatures(), 7)
Review Comment:
ditto `MINIMUM_VERSION.featureLevel()` instead of 7 ?
##########
metadata/src/test/java/org/apache/kafka/controller/FeatureControlManagerTest.java:
##########
@@ -99,10 +100,10 @@ public void testUpdateFeatures() {
FeatureControlManager manager = new FeatureControlManager.Builder().
setQuorumFeatures(features(TestFeatureVersion.FEATURE_NAME, 0, 2)).
setSnapshotRegistry(snapshotRegistry).
- setMetadataVersion(MetadataVersion.IBP_3_3_IV0).
+ setMetadataVersion(MetadataVersion.MINIMUM_VERSION).
build();
snapshotRegistry.idempotentCreateSnapshot(-1);
- assertEquals(new
FinalizedControllerFeatures(Collections.singletonMap("metadata.version",
(short) 4), -1),
+ assertEquals(new
FinalizedControllerFeatures(Collections.singletonMap("metadata.version",
(short) 7), -1),
Review Comment:
ditto `MINIMUM_VERSION.featureLevel()` instead of 7 ?
##########
metadata/src/test/java/org/apache/kafka/controller/FeatureControlManagerTest.java:
##########
@@ -142,11 +143,11 @@ public void testReplay() {
setLogContext(logContext).
setQuorumFeatures(features("foo", 1, 2)).
setSnapshotRegistry(snapshotRegistry).
- setMetadataVersion(MetadataVersion.IBP_3_3_IV0).
+ setMetadataVersion(MetadataVersion.MINIMUM_VERSION).
build();
manager.replay(record);
snapshotRegistry.idempotentCreateSnapshot(123);
- assertEquals(new
FinalizedControllerFeatures(versionMap("metadata.version", 4, "foo", 2), 123),
+ assertEquals(new
FinalizedControllerFeatures(versionMap("metadata.version", 7, "foo", 2), 123),
Review Comment:
ditto `MINIMUM_VERSION.featureLevel()` instead of 7 ?
--
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
