junrao commented on code in PR #18468:
URL: https://github.com/apache/kafka/pull/18468#discussion_r1910885482
##########
core/src/main/scala/kafka/coordinator/group/GroupMetadataManager.scala:
##########
@@ -1112,21 +1109,13 @@ object GroupMetadataManager {
*
* @param groupMetadata current group metadata
* @param assignment the assignment for the rebalancing generation
- * @param metadataVersion the api version
+ * @param version the version to serialize it with, the default is `3`, the
highest supported non-flexible version
+ * until a tagged version is bumped. The default should always be
used outside of tests
Review Comment:
until a tagged version is bumped => until a tagged field is introduced or
the version is bumped
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/classic/ClassicGroup.java:
##########
@@ -1445,7 +1445,7 @@ public void createClassicGroupRecords(
assignments.put(classicGroupMember.memberId(),
classicGroupMember.assignment())
);
- records.add(GroupCoordinatorRecordHelpers.newGroupMetadataRecord(this,
assignments, metadataVersion));
+ records.add(GroupCoordinatorRecordHelpers.newGroupMetadataRecord(this,
assignments));
Review Comment:
`metadataVersion` is no longer used.
##########
core/src/test/scala/unit/kafka/server/ReplicaFetcherThreadTest.scala:
##########
@@ -130,96 +128,12 @@ class ReplicaFetcherThreadTest {
ApiKeys.FETCH.latestVersion(true),
testingVersion.fetchRequestVersion
)
- assertEquals(
- ApiKeys.OFFSET_FOR_LEADER_EPOCH.latestVersion(true),
- testingVersion.offsetForLeaderEpochRequestVersion
- )
assertEquals(
ApiKeys.LIST_OFFSETS.latestVersion(true),
testingVersion.listOffsetRequestVersion
)
}
- @Disabled("KAFKA-18370")
- @Test
- def testFetchLeaderEpochRequestIfLastEpochDefinedForSomePartitions(): Unit =
{
Review Comment:
It's probably better to remove those tests related
OffsetForLeaderEpochRequest until we remove the code in
`AbstractFetcherThread.truncateToEpochEndOffsets()`. This can be done in a
separate jira.
We could remove all tests that depend on old IBPs.
##########
core/src/test/scala/unit/kafka/server/ReplicaFetcherThreadTest.scala:
##########
@@ -48,7 +46,7 @@ import org.junit.jupiter.params.ParameterizedTest
import org.junit.jupiter.params.provider.ValueSource
import org.mockito.ArgumentCaptor
import org.mockito.ArgumentMatchers.{any, anyBoolean, anyLong}
-import org.mockito.Mockito.{mock, never, times, verify, when}
+import org.mockito.Mockito.{mock, times, verify, when}
Review Comment:
We could delete `shouldUseLeaderEndOffsetIfInterBrokerVersionBelow20`.
##########
core/src/test/scala/unit/kafka/log/LogLoaderTest.scala:
##########
@@ -128,7 +127,7 @@ class LogLoaderTest {
brokerTopicStats = new BrokerTopicStats(),
logDirFailureChannel = logDirFailureChannel,
time = time,
- keepPartitionMetadataFile = config.usesTopicId,
+ keepPartitionMetadataFile = true,
Review Comment:
We can probably remove keepPartitionMetadataFile from LogManager. This can
be done in a followup PR.
##########
clients/src/main/java/org/apache/kafka/common/requests/AlterPartitionRequest.java:
##########
@@ -68,14 +68,12 @@ public static class Builder extends
AbstractRequest.Builder<AlterPartitionReques
* @param data The data to be sent. Note that because the version of
the
* request is not known at this time, it is expected that
all
* topics have a topic id and a topic name set.
- * @param canUseTopicIds True if version 2 and above can be used.
*/
- public Builder(AlterPartitionRequestData data, boolean canUseTopicIds)
{
Review Comment:
v2 request is added in 3.3.0. To be consistent, we need to move the MV
baseline to 3.3.
##########
core/src/main/scala/kafka/server/ApiVersionManager.scala:
##########
@@ -150,14 +150,12 @@ class DefaultApiVersionManager(
}
val apiVersions = if (controllerApiVersions.isDefined) {
ApiVersionsResponse.controllerApiVersions(
- finalizedFeatures.metadataVersion().highestSupportedRecordVersion,
Review Comment:
Should we leave this param here to support record format change in the
future?
--
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]
