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

[GitHub] [kafka] machi1990 opened a new pull request, #13664: KAFKA-14961: harden DefaultBackgroundThreadTest.testStartupAndTearDown test

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

   1. Ensures that NPE are not thrown
   2. Ensures that the background thread has been started to avoid flasky assertions failures on isRunning
   3. Add a check that the thread is not running when closed
   
   ### 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] machi1990 commented on pull request #13664: KAFKA-14961: harden DefaultBackgroundThreadTest.testStartupAndTearDown test

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

   Thanks for the review @philipnee Do you know which committer we can tag for this to be reviewed and then merged by them?


-- 
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] philipnee commented on pull request #13664: KAFKA-14961: harden DefaultBackgroundThreadTest.testStartupAndTearDown test

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

   Thanks @vvcephei - I think this is a flaky test?
   
   `Build / JDK 17 and Scala 2.13 / testDescribeStateOfExistingGroupWithRoundRobinAssignor() – kafka.admin.DescribeConsumerGroupTest
   2s
   Skipped - 123`
   


-- 
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] vvcephei commented on pull request #13664: KAFKA-14961: harden DefaultBackgroundThreadTest.testStartupAndTearDown test

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

   Thanks, @philipnee !
   
   I agree it's probably flaky, but I don't see a ticket filed for it. We haven't been very strict recently about this, but I'm reluctant to just forge ahead at this point, given all the mailing list discussions about recent merges breaking the build because they didn't block on failing tests.
   
   Do you mind filing a ticket for this test, and then we can merge it?
   
   (see https://issues.apache.org/jira/browse/KAFKA-8706?jql=project%20%3D%20KAFKA%20AND%20status%20in%20(Open%2C%20%22In%20Progress%22%2C%20Reopened%2C%20%22Patch%20Available%22)%20AND%20text%20~%20%22DescribeConsumerGroupTest%22 )


-- 
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] machi1990 commented on a diff in pull request #13664: KAFKA-14961: harden DefaultBackgroundThreadTest.testStartupAndTearDown test

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


##########
clients/src/test/java/org/apache/kafka/clients/consumer/internals/DefaultBackgroundThreadTest.java:
##########
@@ -89,11 +90,14 @@ public void setup() {
     }
 
     @Test
-    public void testStartupAndTearDown() {
+    public void testStartupAndTearDown() throws InterruptedException {
+        when(coordinatorManager.poll(anyLong())).thenReturn(mockPollCoordinatorResult());
+        when(commitManager.poll(anyLong())).thenReturn(mockPollCommitResult());
         DefaultBackgroundThread backgroundThread = mockBackgroundThread();
         backgroundThread.start();
-        assertTrue(backgroundThread.isRunning());
+        TestUtils.waitForCondition(backgroundThread::isRunning, "Failed awaiting for the background thread to be running");
         backgroundThread.close();
+        assertFalse(backgroundThread.isRunning());

Review Comment:
   Thanks for the review @kirktrue 
   
   It is guaranteed that is running will be false when `close()` is called: https://github.com/apache/kafka/blob/6c3e82f69a081b760a685817332ea0a2f7a2a6a8/clients/src/main/java/org/apache/kafka/clients/consumer/internals/DefaultBackgroundThread.java#L230



-- 
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] machi1990 commented on pull request #13664: KAFKA-14961: harden DefaultBackgroundThreadTest.testStartupAndTearDown test

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

   Hi @vvcephei @showuon can you've a look at this? 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] kirktrue commented on a diff in pull request #13664: KAFKA-14961: harden DefaultBackgroundThreadTest.testStartupAndTearDown test

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


##########
clients/src/test/java/org/apache/kafka/clients/consumer/internals/DefaultBackgroundThreadTest.java:
##########
@@ -89,11 +90,14 @@ public void setup() {
     }
 
     @Test
-    public void testStartupAndTearDown() {
+    public void testStartupAndTearDown() throws InterruptedException {
+        when(coordinatorManager.poll(anyLong())).thenReturn(mockPollCoordinatorResult());
+        when(commitManager.poll(anyLong())).thenReturn(mockPollCommitResult());
         DefaultBackgroundThread backgroundThread = mockBackgroundThread();
         backgroundThread.start();
-        assertTrue(backgroundThread.isRunning());
+        TestUtils.waitForCondition(backgroundThread::isRunning, "Failed awaiting for the background thread to be running");
         backgroundThread.close();
+        assertFalse(backgroundThread.isRunning());

Review Comment:
   Is it possible that `isRunning()` could take some time to return `false` or is it guaranteed to be set by the time `close()` is finished?



-- 
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] machi1990 commented on pull request #13664: KAFKA-14961: harden DefaultBackgroundThreadTest.testStartupAndTearDown test

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

   Thanks @philipnee this should be good to be merged @vvcephei 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] machi1990 commented on pull request #13664: KAFKA-14961: harden DefaultBackgroundThreadTest.testStartupAndTearDown test

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

   @philipnee can you've a look? 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] machi1990 commented on a diff in pull request #13664: KAFKA-14961: harden DefaultBackgroundThreadTest.testStartupAndTearDown test

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


##########
clients/src/test/java/org/apache/kafka/clients/consumer/internals/DefaultBackgroundThreadTest.java:
##########
@@ -89,11 +90,14 @@ public void setup() {
     }
 
     @Test
-    public void testStartupAndTearDown() {
+    public void testStartupAndTearDown() throws InterruptedException {
+        when(coordinatorManager.poll(anyLong())).thenReturn(mockPollCoordinatorResult());
+        when(commitManager.poll(anyLong())).thenReturn(mockPollCommitResult());
         DefaultBackgroundThread backgroundThread = mockBackgroundThread();
         backgroundThread.start();
-        assertTrue(backgroundThread.isRunning());
+        TestUtils.waitForCondition(backgroundThread::isRunning, "Failed awaiting for the background thread to be running");

Review Comment:
   This will wait up to 15s, the default timeout. It should be good enough!



-- 
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] vvcephei commented on pull request #13664: KAFKA-14961: harden DefaultBackgroundThreadTest.testStartupAndTearDown test

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

   I'm re-running the tests to see if we can get a green build.


-- 
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] vvcephei merged pull request #13664: KAFKA-14961: harden DefaultBackgroundThreadTest.testStartupAndTearDown test

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


-- 
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] philipnee commented on pull request #13664: KAFKA-14961: harden DefaultBackgroundThreadTest.testStartupAndTearDown test

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

   Thank you @vvcephei - https://issues.apache.org/jira/browse/KAFKA-14977 is filed 💘 


-- 
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] philipnee commented on pull request #13664: KAFKA-14961: harden DefaultBackgroundThreadTest.testStartupAndTearDown test

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

   Hey @machi1990 - I'll get someone to review this. 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] machi1990 commented on a diff in pull request #13664: KAFKA-14961: harden DefaultBackgroundThreadTest.testStartupAndTearDown test

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


##########
clients/src/test/java/org/apache/kafka/clients/consumer/internals/DefaultBackgroundThreadTest.java:
##########
@@ -89,11 +90,14 @@ public void setup() {
     }
 
     @Test
-    public void testStartupAndTearDown() {
+    public void testStartupAndTearDown() throws InterruptedException {
+        when(coordinatorManager.poll(anyLong())).thenReturn(mockPollCoordinatorResult());
+        when(commitManager.poll(anyLong())).thenReturn(mockPollCommitResult());

Review Comment:
   These stubs call where missing thus causing the NPE as indicated in the JIRA



-- 
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] machi1990 commented on pull request #13664: KAFKA-14961: harden DefaultBackgroundThreadTest.testStartupAndTearDown test

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

   Thanks @philipnee  @vvcephei for the review and merge!


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