You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "dajac (via GitHub)" <gi...@apache.org> on 2023/05/23 13:25:52 UTC

[GitHub] [kafka] dajac opened a new pull request, #13747: MINOR: Fix ListOffsetsRequestTest.testResponseIncludesLeaderEpoch

dajac opened a new pull request, #13747:
URL: https://github.com/apache/kafka/pull/13747

   Fix flaky ListOffsetsRequestTest.testResponseIncludesLeaderEpoch.
   
   ### Committer Checklist (excluded from commit message)
   - [ ] Verify design and implementation 
   - [ ] Verify test coverage and CI build status
   - [ ] Verify documentation (including upgrade notes)
   


-- 
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] dajac merged pull request #13747: MINOR: Fix ListOffsetsRequestTest.testResponseIncludesLeaderEpoch

Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac merged PR #13747:
URL: https://github.com/apache/kafka/pull/13747


-- 
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] satishd commented on pull request #13747: MINOR: Fix ListOffsetsRequestTest.testResponseIncludesLeaderEpoch

Posted by "satishd (via GitHub)" <gi...@apache.org>.
satishd commented on PR #13747:
URL: https://github.com/apache/kafka/pull/13747#issuecomment-1562361733

   Thanks @dajac for fixing this intermittent failure. 


-- 
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] dajac commented on a diff in pull request #13747: MINOR: Fix ListOffsetsRequestTest.testResponseIncludesLeaderEpoch

Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #13747:
URL: https://github.com/apache/kafka/pull/13747#discussion_r1202331207


##########
core/src/test/scala/unit/kafka/server/ListOffsetsRequestTest.scala:
##########
@@ -192,7 +192,7 @@ class ListOffsetsRequestTest extends BaseRequestTest {
     killBroker(firstLeaderId)
     val secondLeaderId = TestUtils.awaitLeaderChange(servers, partition, firstLeaderId)
     // make sure high watermark of new leader has caught up
-    TestUtils.waitUntilTrue(() => sendRequest(secondLeaderId, 0L, -1).errorCode() != Errors.OFFSET_NOT_AVAILABLE.code(),
+    TestUtils.waitUntilTrue(() => sendRequest(secondLeaderId, ListOffsetsRequest.LATEST_TIMESTAMP, -1).errorCode != Errors.OFFSET_NOT_AVAILABLE.code,

Review Comment:
   `OFFSET_NOT_AVAILABLE` is only returned when `LATEST_TIMESTAMP` or `MAX_TIMESTAMP` are used.



-- 
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] showuon commented on a diff in pull request #13747: MINOR: Fix ListOffsetsRequestTest.testResponseIncludesLeaderEpoch

Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon commented on code in PR #13747:
URL: https://github.com/apache/kafka/pull/13747#discussion_r1203646848


##########
core/src/test/scala/unit/kafka/server/ListOffsetsRequestTest.scala:
##########
@@ -192,7 +192,7 @@ class ListOffsetsRequestTest extends BaseRequestTest {
     killBroker(firstLeaderId)
     val secondLeaderId = TestUtils.awaitLeaderChange(servers, partition, firstLeaderId)
     // make sure high watermark of new leader has caught up
-    TestUtils.waitUntilTrue(() => sendRequest(secondLeaderId, 0L, -1).errorCode() != Errors.OFFSET_NOT_AVAILABLE.code(),
+    TestUtils.waitUntilTrue(() => sendRequest(secondLeaderId, ListOffsetsRequest.LATEST_TIMESTAMP, -1).errorCode != Errors.OFFSET_NOT_AVAILABLE.code,

Review Comment:
   So, if my understanding is correct, the test failure is because we are using the the `OFFSET_NOT_AVAILABLE` to identify if the new leader has caught up, but the `OFFSET_NOT_AVAILABLE` error only throws when requesting `LATEST_TIMESTAMP` or a timestamp beyond the last fetchable offset. Since we're using a small number of timestamp, this wait will pass immediately, and cause the below assertion failure with `OFFSET_NOT_AVAILABLE` sometimes. 
   
   Is this correct? If so, this change LGTM! Thanks.



-- 
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] dajac commented on pull request #13747: MINOR: Fix ListOffsetsRequestTest.testResponseIncludesLeaderEpoch

Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on PR #13747:
URL: https://github.com/apache/kafka/pull/13747#issuecomment-1560644863

   @showuon Thanks for your review. Yes, your understanding is correct. This check never worked in fact. The part which is not clear is that this test fails all the time since we merged https://github.com/apache/kafka/pull/13535 but I can't really see a reason.


-- 
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] kamalcph commented on a diff in pull request #13747: MINOR: Fix ListOffsetsRequestTest.testResponseIncludesLeaderEpoch

Posted by "kamalcph (via GitHub)" <gi...@apache.org>.
kamalcph commented on code in PR #13747:
URL: https://github.com/apache/kafka/pull/13747#discussion_r1206227365


##########
core/src/test/scala/unit/kafka/server/ListOffsetsRequestTest.scala:
##########
@@ -192,7 +192,7 @@ class ListOffsetsRequestTest extends BaseRequestTest {
     killBroker(firstLeaderId)
     val secondLeaderId = TestUtils.awaitLeaderChange(servers, partition, firstLeaderId)
     // make sure high watermark of new leader has caught up
-    TestUtils.waitUntilTrue(() => sendRequest(secondLeaderId, 0L, -1).errorCode() != Errors.OFFSET_NOT_AVAILABLE.code(),
+    TestUtils.waitUntilTrue(() => sendRequest(secondLeaderId, ListOffsetsRequest.LATEST_TIMESTAMP, -1).errorCode != Errors.OFFSET_NOT_AVAILABLE.code,

Review Comment:
   Thanks for fixing this!



-- 
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] dajac commented on a diff in pull request #13747: MINOR: Fix ListOffsetsRequestTest.testResponseIncludesLeaderEpoch

Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #13747:
URL: https://github.com/apache/kafka/pull/13747#discussion_r1203652500


##########
core/src/test/scala/unit/kafka/server/ListOffsetsRequestTest.scala:
##########
@@ -192,7 +192,7 @@ class ListOffsetsRequestTest extends BaseRequestTest {
     killBroker(firstLeaderId)
     val secondLeaderId = TestUtils.awaitLeaderChange(servers, partition, firstLeaderId)
     // make sure high watermark of new leader has caught up
-    TestUtils.waitUntilTrue(() => sendRequest(secondLeaderId, 0L, -1).errorCode() != Errors.OFFSET_NOT_AVAILABLE.code(),
+    TestUtils.waitUntilTrue(() => sendRequest(secondLeaderId, ListOffsetsRequest.LATEST_TIMESTAMP, -1).errorCode != Errors.OFFSET_NOT_AVAILABLE.code,

Review Comment:
   Yep. This is correct.



-- 
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] showuon commented on a diff in pull request #13747: MINOR: Fix ListOffsetsRequestTest.testResponseIncludesLeaderEpoch

Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon commented on code in PR #13747:
URL: https://github.com/apache/kafka/pull/13747#discussion_r1203646848


##########
core/src/test/scala/unit/kafka/server/ListOffsetsRequestTest.scala:
##########
@@ -192,7 +192,7 @@ class ListOffsetsRequestTest extends BaseRequestTest {
     killBroker(firstLeaderId)
     val secondLeaderId = TestUtils.awaitLeaderChange(servers, partition, firstLeaderId)
     // make sure high watermark of new leader has caught up
-    TestUtils.waitUntilTrue(() => sendRequest(secondLeaderId, 0L, -1).errorCode() != Errors.OFFSET_NOT_AVAILABLE.code(),
+    TestUtils.waitUntilTrue(() => sendRequest(secondLeaderId, ListOffsetsRequest.LATEST_TIMESTAMP, -1).errorCode != Errors.OFFSET_NOT_AVAILABLE.code,

Review Comment:
   So, if my understanding is correct, the test failure is because we are using the the `OFFSET_NOT_AVAILABLE` to identify if the new leader has caught up, but the `OFFSET_NOT_AVAILABLE` error only throws when requesting `LATEST_TIMESTAMP` or a timestamp beyond the last fetchable offset. Since we were using a small number of timestamp, this wait will pass immediately, and cause the below assertion failure with `OFFSET_NOT_AVAILABLE` sometimes. 
   
   Is this correct? If so, this change LGTM! Thanks.



-- 
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