dajac commented on a change in pull request #9540:
URL: https://github.com/apache/kafka/pull/9540#discussion_r515806990
##########
File path:
clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java
##########
@@ -1261,10 +1261,9 @@ private ListOffsetRequest createListOffsetRequest(int
version) {
} else if (version >= 2 && version <= 5) {
ListOffsetPartition partition = new ListOffsetPartition()
.setPartitionIndex(0)
- .setTimestamp(1000000L);
- if (version >= 4) {
- partition.setCurrentLeaderEpoch(5);
- }
+ .setTimestamp(1000000L)
+ .setCurrentLeaderEpoch(5);
Review comment:
I suggest to set current leader epoch for all the versions as we have
made it ignorable. We set it all the time in the replica fetcher so the test
would be aligned with what we do.
##########
File path: core/src/test/scala/unit/kafka/server/ListOffsetsRequestTest.scala
##########
@@ -180,7 +183,8 @@ class ListOffsetsRequestTest extends BaseRequestTest {
TestUtils.generateAndProduceMessages(servers, topic, 10)
- assertEquals((-1L, -1), fetchOffsetAndEpoch(firstLeaderId, 0L, 0))
+ assertEquals((0L, -1), fetchOffsetAndEpoch(firstLeaderId,
ListOffsetRequest.EARLIEST_TIMESTAMP, 0))
+ assertEquals((10L, -1), fetchOffsetAndEpoch(firstLeaderId,
ListOffsetRequest.LATEST_TIMESTAMP, 0))
Review comment:
Would it make sense to test `EARLIEST_TIMESTAMP`, `LATEST_TIMESTAMP`,
and `0L` cases for all the versions?
##########
File path: core/src/test/scala/unit/kafka/server/ListOffsetsRequestTest.scala
##########
@@ -143,7 +143,13 @@ class ListOffsetsRequestTest extends BaseRequestTest {
val partitionData = response.topics.asScala.find(_.name == topic).get
.partitions.asScala.find(_.partitionIndex == partition.partition).get
- (partitionData.offset, partitionData.leaderEpoch)
+ if (version == 0) {
+ if (partitionData.oldStyleOffsets().isEmpty)
+ (-1, partitionData.leaderEpoch)
+ else
+ (partitionData.oldStyleOffsets().asScala.head,
partitionData.leaderEpoch)
Review comment:
nit: We could use `headOption.getOrElse(-1)` and avoid the extra
`if/else`.
##########
File path: core/src/test/scala/unit/kafka/server/ListOffsetsRequestTest.scala
##########
@@ -180,11 +186,21 @@ class ListOffsetsRequestTest extends BaseRequestTest {
TestUtils.generateAndProduceMessages(servers, topic, 10)
- assertEquals((-1L, -1), fetchOffsetAndEpoch(firstLeaderId, 0L, 0))
- assertEquals((0L, -1), fetchOffsetAndEpoch(firstLeaderId, 0L, 1))
- assertEquals((0L, -1), fetchOffsetAndEpoch(firstLeaderId, 0L, 2))
- assertEquals((0L, -1), fetchOffsetAndEpoch(firstLeaderId, 0L, 3))
- assertEquals((0L, 0), fetchOffsetAndEpoch(firstLeaderId, 0L, 4))
+ for (version <- ApiKeys.LIST_OFFSETS.oldestVersion to
ApiKeys.LIST_OFFSETS.latestVersion) {
Review comment:
nit: Now that we test all versions, could we rename to test to
`testResponseDefaultOffsetAndLeaderEpochForAllVersions` in order to stay
consistent?
##########
File path: core/src/test/scala/unit/kafka/server/ListOffsetsRequestTest.scala
##########
@@ -180,11 +186,21 @@ class ListOffsetsRequestTest extends BaseRequestTest {
TestUtils.generateAndProduceMessages(servers, topic, 10)
- assertEquals((-1L, -1), fetchOffsetAndEpoch(firstLeaderId, 0L, 0))
- assertEquals((0L, -1), fetchOffsetAndEpoch(firstLeaderId, 0L, 1))
- assertEquals((0L, -1), fetchOffsetAndEpoch(firstLeaderId, 0L, 2))
- assertEquals((0L, -1), fetchOffsetAndEpoch(firstLeaderId, 0L, 3))
- assertEquals((0L, 0), fetchOffsetAndEpoch(firstLeaderId, 0L, 4))
+ for (version <- ApiKeys.LIST_OFFSETS.oldestVersion to
ApiKeys.LIST_OFFSETS.latestVersion) {
+ if (version == 0) {
+ assertEquals((-1L, -1), fetchOffsetAndEpoch(firstLeaderId, 0L,
version.toShort))
+ assertEquals((0L, -1), fetchOffsetAndEpoch(firstLeaderId,
ListOffsetRequest.EARLIEST_TIMESTAMP, version.toShort))
+ assertEquals((10L, -1), fetchOffsetAndEpoch(firstLeaderId,
ListOffsetRequest.LATEST_TIMESTAMP, version.toShort))
+ } else if (version >= 1 && version <=3) {
+ assertEquals((0L, -1), fetchOffsetAndEpoch(firstLeaderId, 0L,
version.toShort))
+ assertEquals((0L, -1), fetchOffsetAndEpoch(firstLeaderId,
ListOffsetRequest.EARLIEST_TIMESTAMP, version.toShort))
+ assertEquals((10L, -1), fetchOffsetAndEpoch(firstLeaderId,
ListOffsetRequest.LATEST_TIMESTAMP, version.toShort))
+ } else if (version >=4) {
Review comment:
nit: Missing space before `4`.
##########
File path: core/src/test/scala/unit/kafka/server/ListOffsetsRequestTest.scala
##########
@@ -180,11 +186,21 @@ class ListOffsetsRequestTest extends BaseRequestTest {
TestUtils.generateAndProduceMessages(servers, topic, 10)
- assertEquals((-1L, -1), fetchOffsetAndEpoch(firstLeaderId, 0L, 0))
- assertEquals((0L, -1), fetchOffsetAndEpoch(firstLeaderId, 0L, 1))
- assertEquals((0L, -1), fetchOffsetAndEpoch(firstLeaderId, 0L, 2))
- assertEquals((0L, -1), fetchOffsetAndEpoch(firstLeaderId, 0L, 3))
- assertEquals((0L, 0), fetchOffsetAndEpoch(firstLeaderId, 0L, 4))
+ for (version <- ApiKeys.LIST_OFFSETS.oldestVersion to
ApiKeys.LIST_OFFSETS.latestVersion) {
+ if (version == 0) {
+ assertEquals((-1L, -1), fetchOffsetAndEpoch(firstLeaderId, 0L,
version.toShort))
+ assertEquals((0L, -1), fetchOffsetAndEpoch(firstLeaderId,
ListOffsetRequest.EARLIEST_TIMESTAMP, version.toShort))
+ assertEquals((10L, -1), fetchOffsetAndEpoch(firstLeaderId,
ListOffsetRequest.LATEST_TIMESTAMP, version.toShort))
+ } else if (version >= 1 && version <=3) {
Review comment:
nit: Missing space before `3`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
