chia7712 commented on code in PR #18468:
URL: https://github.com/apache/kafka/pull/18468#discussion_r1909751077
##########
group-coordinator/src/test/java/org/apache/kafka/coordinator/group/GroupMetadataManagerTestContext.java:
##########
@@ -238,7 +238,7 @@ public static CoordinatorRecord newGroupMetadataRecord(
),
new ApiMessageAndVersion(
value,
- metadataVersion.groupMetadataValueVersion()
Review Comment:
Could you please remove variable `MetadataVersion metadataVersion` as it is
unused now?
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupCoordinatorRecordHelpers.java:
##########
@@ -552,18 +551,14 @@ public static CoordinatorRecord
newEmptyGroupMetadataRecord(
* @param topic The topic name.
* @param partitionId The partition id.
* @param offsetAndMetadata The offset and metadata.
- * @param metadataVersion The metadata version.
* @return The record.
*/
public static CoordinatorRecord newOffsetCommitRecord(
String groupId,
String topic,
int partitionId,
- OffsetAndMetadata offsetAndMetadata,
- MetadataVersion metadataVersion
+ OffsetAndMetadata offsetAndMetadata
) {
- short version =
metadataVersion.offsetCommitValueVersion(offsetAndMetadata.expireTimestampMs.isPresent());
Review Comment:
It seems we need to use version=1 in order to keep "expireTimestamp".
otherwise, in v3 `expireTimestamp` will be ignored in serialization.
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupCoordinatorShard.java:
##########
@@ -731,7 +731,6 @@ private void cancelClassicGroupSizeCounter() {
public void onLoaded(MetadataImage newImage) {
MetadataDelta emptyDelta = new MetadataDelta(newImage);
groupMetadataManager.onNewMetadataImage(newImage, emptyDelta);
- offsetMetadataManager.onNewMetadataImage(newImage, emptyDelta);
Review Comment:
I guess we don't need to clean up `GroupMetadataManager.scala`, as 4.0 uses
a new coordinator by default?
##########
clients/src/test/java/org/apache/kafka/common/requests/OffsetsForLeaderEpochRequestTest.java:
##########
@@ -40,20 +40,4 @@ public void testForConsumerRequiresVersion3() {
assertEquals(OffsetsForLeaderEpochRequest.CONSUMER_REPLICA_ID,
request.replicaId());
}
}
-
- @Test
- public void testDefaultReplicaId() {
- for (short version : ApiKeys.OFFSET_FOR_LEADER_EPOCH.allVersions()) {
- int replicaId = 1;
- OffsetsForLeaderEpochRequest.Builder builder =
OffsetsForLeaderEpochRequest.Builder.forFollower(
- version, new OffsetForLeaderTopicCollection(), replicaId);
- OffsetsForLeaderEpochRequest request = builder.build();
- OffsetsForLeaderEpochRequest parsed =
OffsetsForLeaderEpochRequest.parse(request.serialize(), version);
- if (version < 3)
-
assertEquals(OffsetsForLeaderEpochRequest.DEBUGGING_REPLICA_ID,
parsed.replicaId());
- else
- assertEquals(replicaId, parsed.replicaId());
Review Comment:
Should we keep the test for version=4 ?
--
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]
