You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/05/31 03:35:18 UTC

[GitHub] [pulsar] lhotari opened a new pull request, #15851: [Tests] Fix flaky MultiTopicsConsumerImplTest

lhotari opened a new pull request, #15851:
URL: https://github.com/apache/pulsar/pull/15851

   Fixes #15850
   
   ### Motivation
   
   See #15850 for details. MultiTopicsConsumerImplTest.testParallelSubscribeAsync is flaky and several problems were observed in the test. This PR attempts to fix the issues.
   
   ### Modifications
   
   - add preProcessSchemaBeforeSubscribe mocking to prevent `java.lang.NullPointerException: Cannot invoke "org.apache.pulsar.client.api.Schema.getSchemaInfo()" because "this.schema" is null` NPEs.
   - the timeout in testParallelSubscribeAsync method seems to happen because testGetStats continues asynchronously in the background. To fix this issue:
     - use arbitrary local ip + port for testGetStats (the test doesn't require a real connection, this should be improved)
     - close the resources synchronously in testGetStats 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: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] lhotari commented on a diff in pull request #15851: [Tests] Fix flaky MultiTopicsConsumerImplTest

Posted by GitBox <gi...@apache.org>.
lhotari commented on code in PR #15851:
URL: https://github.com/apache/pulsar/pull/15851#discussion_r885479723


##########
pulsar-client/src/test/java/org/apache/pulsar/client/impl/MultiTopicsConsumerImplTest.java:
##########
@@ -112,9 +112,13 @@ public void testGetStats() throws Exception {
 
         MultiTopicsConsumerImpl impl = new MultiTopicsConsumerImpl(
             clientImpl, consumerConfData,
-            executorProvider, null, null, null, true);
+            executorProvider, null, Schema.BYTES, null, true);
 
         impl.getStats();
+
+        clientImpl.close();
+        executorProvider.shutdownNow();
+        eventLoopGroup.shutdownGracefully().get();

Review Comment:
   > and then we can remove the @cleanup annotations ?
   
   it's true that there is duplication. however, `@Cleanup` automatically adds try-finally. 



-- 
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: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] lhotari commented on pull request #15851: [Tests] Fix flaky MultiTopicsConsumerImplTest

Posted by GitBox <gi...@apache.org>.
lhotari commented on PR #15851:
URL: https://github.com/apache/pulsar/pull/15851#issuecomment-1141924004

   I included the commit for #15777 since it reduces flakiness. When we merge #15777 before this one, it won't be included in the final merge result to master. Please take this into consideration when reviewing 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: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] lhotari merged pull request #15851: [Tests] Fix flaky MultiTopicsConsumerImplTest

Posted by GitBox <gi...@apache.org>.
lhotari merged PR #15851:
URL: https://github.com/apache/pulsar/pull/15851


-- 
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: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] lhotari commented on pull request #15851: [Tests] Fix flaky MultiTopicsConsumerImplTest

Posted by GitBox <gi...@apache.org>.
lhotari commented on PR #15851:
URL: https://github.com/apache/pulsar/pull/15851#issuecomment-1141983825

   Marking this as draft until #15777 is merged.


-- 
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: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] lhotari commented on a diff in pull request #15851: [Tests] Fix flaky MultiTopicsConsumerImplTest

Posted by GitBox <gi...@apache.org>.
lhotari commented on code in PR #15851:
URL: https://github.com/apache/pulsar/pull/15851#discussion_r885478682


##########
pulsar-client/src/test/java/org/apache/pulsar/client/impl/MultiTopicsConsumerImplTest.java:
##########
@@ -112,9 +112,13 @@ public void testGetStats() throws Exception {
 
         MultiTopicsConsumerImpl impl = new MultiTopicsConsumerImpl(
             clientImpl, consumerConfData,
-            executorProvider, null, null, null, true);
+            executorProvider, null, Schema.BYTES, null, true);
 
         impl.getStats();
+
+        clientImpl.close();
+        executorProvider.shutdownNow();
+        eventLoopGroup.shutdownGracefully().get();

Review Comment:
   > sync() ?
   
   yes. I have never seen `eventLoopGroup.shutdownGracefully().get()` get stuck. 



-- 
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: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] nicoloboschi commented on pull request #15851: [Tests] Fix flaky MultiTopicsConsumerImplTest

Posted by GitBox <gi...@apache.org>.
nicoloboschi commented on PR #15851:
URL: https://github.com/apache/pulsar/pull/15851#issuecomment-1141977391

   I approved the pull, I agree that it's better to go ahead 


-- 
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: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] lhotari commented on a diff in pull request #15851: [Tests] Fix flaky MultiTopicsConsumerImplTest

Posted by GitBox <gi...@apache.org>.
lhotari commented on code in PR #15851:
URL: https://github.com/apache/pulsar/pull/15851#discussion_r885480279


##########
pulsar-client/src/test/java/org/apache/pulsar/client/impl/MultiTopicsConsumerImplTest.java:
##########
@@ -112,9 +112,13 @@ public void testGetStats() throws Exception {
 
         MultiTopicsConsumerImpl impl = new MultiTopicsConsumerImpl(
             clientImpl, consumerConfData,
-            executorProvider, null, null, null, true);
+            executorProvider, null, Schema.BYTES, null, true);
 
         impl.getStats();
+
+        clientImpl.close();
+        executorProvider.shutdownNow();

Review Comment:
   you are right about that. however it's not currently a problem in this test.



-- 
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: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] lhotari commented on pull request #15851: [Tests] Fix flaky MultiTopicsConsumerImplTest

Posted by GitBox <gi...@apache.org>.
lhotari commented on PR #15851:
URL: https://github.com/apache/pulsar/pull/15851#issuecomment-1141971094

   @nicoloboschi thanks for the review. you are right about the comments, but none of the issues are crucial. The PR fixes an extremely flaky issue and I'd rather have it merged as is if there aren't any blockers. We can follow up on the improvements once CI is in better shape.


-- 
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: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] nicoloboschi commented on a diff in pull request #15851: [Tests] Fix flaky MultiTopicsConsumerImplTest

Posted by GitBox <gi...@apache.org>.
nicoloboschi commented on code in PR #15851:
URL: https://github.com/apache/pulsar/pull/15851#discussion_r885444699


##########
pulsar-client/src/test/java/org/apache/pulsar/client/impl/MultiTopicsConsumerImplTest.java:
##########
@@ -112,9 +112,13 @@ public void testGetStats() throws Exception {
 
         MultiTopicsConsumerImpl impl = new MultiTopicsConsumerImpl(
             clientImpl, consumerConfData,
-            executorProvider, null, null, null, true);
+            executorProvider, null, Schema.BYTES, null, true);
 
         impl.getStats();
+
+        clientImpl.close();
+        executorProvider.shutdownNow();
+        eventLoopGroup.shutdownGracefully().get();

Review Comment:
   sync() ? 



##########
pulsar-client/src/test/java/org/apache/pulsar/client/impl/MultiTopicsConsumerImplTest.java:
##########
@@ -112,9 +112,13 @@ public void testGetStats() throws Exception {
 
         MultiTopicsConsumerImpl impl = new MultiTopicsConsumerImpl(
             clientImpl, consumerConfData,
-            executorProvider, null, null, null, true);
+            executorProvider, null, Schema.BYTES, null, true);
 
         impl.getStats();
+
+        clientImpl.close();
+        executorProvider.shutdownNow();

Review Comment:
   it is better to wait for threads termination here, you can use the GracefulShutdown utility



##########
pulsar-client/src/test/java/org/apache/pulsar/client/impl/MultiTopicsConsumerImplTest.java:
##########
@@ -112,9 +112,13 @@ public void testGetStats() throws Exception {
 
         MultiTopicsConsumerImpl impl = new MultiTopicsConsumerImpl(
             clientImpl, consumerConfData,
-            executorProvider, null, null, null, true);
+            executorProvider, null, Schema.BYTES, null, true);
 
         impl.getStats();
+
+        clientImpl.close();
+        executorProvider.shutdownNow();
+        eventLoopGroup.shutdownGracefully().get();

Review Comment:
   and then we can remove the 
   @Cleanup annotations  ? 



-- 
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: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org