You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "kowshik (via GitHub)" <gi...@apache.org> on 2023/02/17 17:06:47 UTC
[GitHub] [kafka] kowshik opened a new pull request, #13272: MINOR: Add missing unit tests for {Local|Remote}LeaderEndpoint classes
kowshik opened a new pull request, #13272:
URL: https://github.com/apache/kafka/pull/13272
I've added unit tests that were previously missing for the `LeaderEndpoint.fetchEpochEndOffsets()` public method.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] kowshik commented on a diff in pull request #13272: MINOR: Add missing unit tests for {Local|Remote}LeaderEndpoint classes
Posted by "kowshik (via GitHub)" <gi...@apache.org>.
kowshik commented on code in PR #13272:
URL: https://github.com/apache/kafka/pull/13272#discussion_r1110637054
##########
core/src/test/scala/kafka/server/LocalLeaderEndPointTest.scala:
##########
@@ -118,6 +120,46 @@ class LocalLeaderEndPointTest {
assertEquals((4, 3L), endPoint.fetchEarliestLocalOffset(topicPartition, currentLeaderEpoch = 7))
}
+ @Test
+ def testFetchEpochEndOffsets(): Unit = {
+ appendRecords(replicaManager, topicPartition, records)
+ .onFire(response => assertEquals(Errors.NONE, response.error))
+
+ var result = endPoint.fetchEpochEndOffsets(Map(
+ topicPartition -> new OffsetForLeaderPartition()
+ .setPartition(topicPartition.partition)
+ .setLeaderEpoch(0)))
+
+ var expected = Map(
+ topicPartition -> new EpochEndOffset()
+ .setPartition(topicPartition.partition)
+ .setErrorCode(Errors.NONE.code)
+ .setLeaderEpoch(0)
+ .setEndOffset(3L))
+
+ assertEquals(expected, result)
+
+ // Change leader epoch and end offset, and verify the behavior again.
+ val leaderAndIsrRequest = buildLeaderAndIsrRequest(leaderEpoch = 4)
+ replicaManager.becomeLeaderOrFollower(0, leaderAndIsrRequest, (_, _) => ())
+ appendRecords(replicaManager, topicPartition, records)
+ .onFire(response => assertEquals(Errors.NONE, response.error))
+
+ result = endPoint.fetchEpochEndOffsets(Map(
+ topicPartition -> new OffsetForLeaderPartition()
+ .setPartition(topicPartition.partition)
+ .setLeaderEpoch(4)))
+
+ expected = Map(
+ topicPartition -> new EpochEndOffset()
+ .setPartition(topicPartition.partition)
+ .setErrorCode(Errors.NONE.code)
+ .setLeaderEpoch(4)
+ .setEndOffset(6L))
+
+ assertEquals(expected, result)
Review Comment:
Done.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] kowshik commented on pull request #13272: MINOR: Add missing unit tests for {Local|Remote}LeaderEndpoint classes
Posted by "kowshik (via GitHub)" <gi...@apache.org>.
kowshik commented on PR #13272:
URL: https://github.com/apache/kafka/pull/13272#issuecomment-1435489782
@junrao Thanks for the review! I've addressed the comment in bc94d6d3a7a541e5d84d735b45ea4435f63a0974.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] junrao commented on a diff in pull request #13272: MINOR: Add missing unit tests for {Local|Remote}LeaderEndpoint classes
Posted by "junrao (via GitHub)" <gi...@apache.org>.
junrao commented on code in PR #13272:
URL: https://github.com/apache/kafka/pull/13272#discussion_r1113490472
##########
core/src/test/scala/kafka/server/LocalLeaderEndPointTest.scala:
##########
@@ -118,6 +120,61 @@ class LocalLeaderEndPointTest {
assertEquals((4, 3L), endPoint.fetchEarliestLocalOffset(topicPartition, currentLeaderEpoch = 7))
}
+ @Test
+ def testFetchEpochEndOffsets(): Unit = {
+ appendRecords(replicaManager, topicPartition, records)
+ .onFire(response => assertEquals(Errors.NONE, response.error))
+
+ var result = endPoint.fetchEpochEndOffsets(Map(
+ topicPartition -> new OffsetForLeaderPartition()
+ .setPartition(topicPartition.partition)
+ .setLeaderEpoch(0)))
+
+ var expected = Map(
+ topicPartition -> new EpochEndOffset()
+ .setPartition(topicPartition.partition)
+ .setErrorCode(Errors.NONE.code)
+ .setLeaderEpoch(0)
+ .setEndOffset(3L))
+
+ assertEquals(expected, result)
+
+ // Change leader epoch and end offset, and verify the behavior again.
+ val leaderAndIsrRequest = buildLeaderAndIsrRequest(leaderEpoch = 4)
+ replicaManager.becomeLeaderOrFollower(0, leaderAndIsrRequest, (_, _) => ())
+ appendRecords(replicaManager, topicPartition, records)
+ .onFire(response => assertEquals(Errors.NONE, response.error))
+
+ result = endPoint.fetchEpochEndOffsets(Map(
+ topicPartition -> new OffsetForLeaderPartition()
+ .setPartition(topicPartition.partition)
+ .setLeaderEpoch(4)))
+
+ expected = Map(
+ topicPartition -> new EpochEndOffset()
+ .setPartition(topicPartition.partition)
+ .setErrorCode(Errors.NONE.code)
+ .setLeaderEpoch(4)
+ .setEndOffset(6L))
+
+ assertEquals(expected, result)
+
+ // Check missing epoch
+ result = endPoint.fetchEpochEndOffsets(Map(
+ topicPartition -> new OffsetForLeaderPartition()
+ .setPartition(topicPartition.partition)
+ .setLeaderEpoch(5)))
Review Comment:
Could we add another test for the missing epoch in the middle like 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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] kowshik commented on a diff in pull request #13272: MINOR: Add missing unit tests for {Local|Remote}LeaderEndpoint classes
Posted by "kowshik (via GitHub)" <gi...@apache.org>.
kowshik commented on code in PR #13272:
URL: https://github.com/apache/kafka/pull/13272#discussion_r1113579561
##########
core/src/test/scala/kafka/server/LocalLeaderEndPointTest.scala:
##########
@@ -118,6 +120,61 @@ class LocalLeaderEndPointTest {
assertEquals((4, 3L), endPoint.fetchEarliestLocalOffset(topicPartition, currentLeaderEpoch = 7))
}
+ @Test
+ def testFetchEpochEndOffsets(): Unit = {
+ appendRecords(replicaManager, topicPartition, records)
+ .onFire(response => assertEquals(Errors.NONE, response.error))
+
+ var result = endPoint.fetchEpochEndOffsets(Map(
+ topicPartition -> new OffsetForLeaderPartition()
+ .setPartition(topicPartition.partition)
+ .setLeaderEpoch(0)))
+
+ var expected = Map(
+ topicPartition -> new EpochEndOffset()
+ .setPartition(topicPartition.partition)
+ .setErrorCode(Errors.NONE.code)
+ .setLeaderEpoch(0)
+ .setEndOffset(3L))
+
+ assertEquals(expected, result)
+
+ // Change leader epoch and end offset, and verify the behavior again.
+ val leaderAndIsrRequest = buildLeaderAndIsrRequest(leaderEpoch = 4)
+ replicaManager.becomeLeaderOrFollower(0, leaderAndIsrRequest, (_, _) => ())
+ appendRecords(replicaManager, topicPartition, records)
+ .onFire(response => assertEquals(Errors.NONE, response.error))
+
+ result = endPoint.fetchEpochEndOffsets(Map(
+ topicPartition -> new OffsetForLeaderPartition()
+ .setPartition(topicPartition.partition)
+ .setLeaderEpoch(4)))
+
+ expected = Map(
+ topicPartition -> new EpochEndOffset()
+ .setPartition(topicPartition.partition)
+ .setErrorCode(Errors.NONE.code)
+ .setLeaderEpoch(4)
+ .setEndOffset(6L))
+
+ assertEquals(expected, result)
+
+ // Check missing epoch
+ result = endPoint.fetchEpochEndOffsets(Map(
+ topicPartition -> new OffsetForLeaderPartition()
+ .setPartition(topicPartition.partition)
+ .setLeaderEpoch(5)))
Review Comment:
Done in 8fdf69e.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] kowshik commented on pull request #13272: MINOR: Add missing unit tests for {Local|Remote}LeaderEndpoint classes
Posted by "kowshik (via GitHub)" <gi...@apache.org>.
kowshik commented on PR #13272:
URL: https://github.com/apache/kafka/pull/13272#issuecomment-1439108799
@junrao Thanks for the review! I've addressed the comment, PR is ready for another pass. The CI test failures look unrelated to this PR.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] junrao commented on a diff in pull request #13272: MINOR: Add missing unit tests for {Local|Remote}LeaderEndpoint classes
Posted by "junrao (via GitHub)" <gi...@apache.org>.
junrao commented on code in PR #13272:
URL: https://github.com/apache/kafka/pull/13272#discussion_r1110288858
##########
core/src/test/scala/kafka/server/LocalLeaderEndPointTest.scala:
##########
@@ -118,6 +120,46 @@ class LocalLeaderEndPointTest {
assertEquals((4, 3L), endPoint.fetchEarliestLocalOffset(topicPartition, currentLeaderEpoch = 7))
}
+ @Test
+ def testFetchEpochEndOffsets(): Unit = {
+ appendRecords(replicaManager, topicPartition, records)
+ .onFire(response => assertEquals(Errors.NONE, response.error))
+
+ var result = endPoint.fetchEpochEndOffsets(Map(
+ topicPartition -> new OffsetForLeaderPartition()
+ .setPartition(topicPartition.partition)
+ .setLeaderEpoch(0)))
+
+ var expected = Map(
+ topicPartition -> new EpochEndOffset()
+ .setPartition(topicPartition.partition)
+ .setErrorCode(Errors.NONE.code)
+ .setLeaderEpoch(0)
+ .setEndOffset(3L))
+
+ assertEquals(expected, result)
+
+ // Change leader epoch and end offset, and verify the behavior again.
+ val leaderAndIsrRequest = buildLeaderAndIsrRequest(leaderEpoch = 4)
+ replicaManager.becomeLeaderOrFollower(0, leaderAndIsrRequest, (_, _) => ())
+ appendRecords(replicaManager, topicPartition, records)
+ .onFire(response => assertEquals(Errors.NONE, response.error))
+
+ result = endPoint.fetchEpochEndOffsets(Map(
+ topicPartition -> new OffsetForLeaderPartition()
+ .setPartition(topicPartition.partition)
+ .setLeaderEpoch(4)))
+
+ expected = Map(
+ topicPartition -> new EpochEndOffset()
+ .setPartition(topicPartition.partition)
+ .setErrorCode(Errors.NONE.code)
+ .setLeaderEpoch(4)
+ .setEndOffset(6L))
+
+ assertEquals(expected, result)
Review Comment:
Could we also test a missing epoch like 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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] junrao merged pull request #13272: MINOR: Add missing unit tests for {Local|Remote}LeaderEndpoint classes
Posted by "junrao (via GitHub)" <gi...@apache.org>.
junrao merged PR #13272:
URL: https://github.com/apache/kafka/pull/13272
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] kowshik commented on pull request #13272: MINOR: Add missing unit tests for {Local|Remote}LeaderEndpoint classes
Posted by "kowshik (via GitHub)" <gi...@apache.org>.
kowshik commented on PR #13272:
URL: https://github.com/apache/kafka/pull/13272#issuecomment-1434942445
Hi @junrao / @satishd / @mattwong949 -- Please could you help review this PR?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org