You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by GitBox <gi...@apache.org> on 2020/06/10 20:34:20 UTC

[GitHub] [storm] Ethanlm opened a new pull request #3286: [STORM-3651] Give producerTasks in ExecutorTransferMultiThreadingTesttestExecutorTransfer more time to finish

Ethanlm opened a new pull request #3286:
URL: https://github.com/apache/storm/pull/3286


   ## What is the purpose of the change
   
   Give producerTasks more time to finish to avoid unnecessary test failures in case of lack of cpu resources.
   
   ## How was the change tested
   
   Ran the unit 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.

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

[GitHub] [storm] bipinprasad commented on a change in pull request #3286: [STORM-3651] Give producerTasks in ExecutorTransferMultiThreadingTest.testExecutorTransfer more time to finish

Posted by GitBox <gi...@apache.org>.
bipinprasad commented on a change in pull request #3286:
URL: https://github.com/apache/storm/pull/3286#discussion_r438986052



##########
File path: storm-client/test/jvm/org/apache/storm/executor/ExecutorTransferMultiThreadingTest.java
##########
@@ -126,14 +126,15 @@ public void testExecutorTransfer() throws InterruptedException {
         }
 
         //give producers enough time to insert messages into the queue
-        executorService.awaitTermination(100, TimeUnit.MILLISECONDS);
+        executorService.awaitTermination(1000, TimeUnit.MILLISECONDS);

Review comment:
       As you said, this is harmless for testing. Hopefully the core performance has not been significantly affected. 

##########
File path: storm-client/test/jvm/org/apache/storm/executor/ExecutorTransferMultiThreadingTest.java
##########
@@ -126,14 +126,15 @@ public void testExecutorTransfer() throws InterruptedException {
         }
 
         //give producers enough time to insert messages into the queue
-        executorService.awaitTermination(100, TimeUnit.MILLISECONDS);
+        executorService.awaitTermination(1000, TimeUnit.MILLISECONDS);

Review comment:
       As you said, this is harmless for testing. Hopefully the core performance has not been significantly affected. 

##########
File path: storm-client/test/jvm/org/apache/storm/executor/ExecutorTransferMultiThreadingTest.java
##########
@@ -126,14 +126,15 @@ public void testExecutorTransfer() throws InterruptedException {
         }
 
         //give producers enough time to insert messages into the queue
-        executorService.awaitTermination(100, TimeUnit.MILLISECONDS);
+        executorService.awaitTermination(1000, TimeUnit.MILLISECONDS);

Review comment:
       As you said, this is harmless for testing. Hopefully the core performance has not been significantly affected. 




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [storm] Ethanlm merged pull request #3286: [STORM-3651] Give producerTasks in ExecutorTransferMultiThreadingTest.testExecutorTransfer more time to finish

Posted by GitBox <gi...@apache.org>.
Ethanlm merged pull request #3286:
URL: https://github.com/apache/storm/pull/3286


   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [storm] Ethanlm commented on a change in pull request #3286: [STORM-3651] Give producerTasks in ExecutorTransferMultiThreadingTest.testExecutorTransfer more time to finish

Posted by GitBox <gi...@apache.org>.
Ethanlm commented on a change in pull request #3286:
URL: https://github.com/apache/storm/pull/3286#discussion_r439648844



##########
File path: storm-client/test/jvm/org/apache/storm/executor/ExecutorTransferMultiThreadingTest.java
##########
@@ -126,14 +126,15 @@ public void testExecutorTransfer() throws InterruptedException {
         }
 
         //give producers enough time to insert messages into the queue
-        executorService.awaitTermination(100, TimeUnit.MILLISECONDS);
+        executorService.awaitTermination(1000, TimeUnit.MILLISECONDS);

Review comment:
       Yeah, we haven't changed anything related to core performance. This is a new unit test. It works well in my laptop but fails sometimes on travis. That is mostly an environment difference. 




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [storm] Ethanlm commented on pull request #3286: [STORM-3651] Give producerTasks in ExecutorTransferMultiThreadingTest.testExecutorTransfer more time to finish

Posted by GitBox <gi...@apache.org>.
Ethanlm commented on pull request #3286:
URL: https://github.com/apache/storm/pull/3286#issuecomment-644279794


   cherry-picked to 2.2.x-branch https://github.com/apache/storm/commit/3d969c4d63be9fe2759f44af609d5d7a88d6a30a


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [storm] bipinprasad commented on a change in pull request #3286: [STORM-3651] Give producerTasks in ExecutorTransferMultiThreadingTest.testExecutorTransfer more time to finish

Posted by GitBox <gi...@apache.org>.
bipinprasad commented on a change in pull request #3286:
URL: https://github.com/apache/storm/pull/3286#discussion_r438930049



##########
File path: storm-client/test/jvm/org/apache/storm/executor/ExecutorTransferMultiThreadingTest.java
##########
@@ -126,14 +126,15 @@ public void testExecutorTransfer() throws InterruptedException {
         }
 
         //give producers enough time to insert messages into the queue
-        executorService.awaitTermination(100, TimeUnit.MILLISECONDS);
+        executorService.awaitTermination(1000, TimeUnit.MILLISECONDS);

Review comment:
       Observation: that is really a long time for 10 messages. Wonder why it is that slow.




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [storm] Ethanlm commented on a change in pull request #3286: [STORM-3651] Give producerTasks in ExecutorTransferMultiThreadingTest.testExecutorTransfer more time to finish

Posted by GitBox <gi...@apache.org>.
Ethanlm commented on a change in pull request #3286:
URL: https://github.com/apache/storm/pull/3286#discussion_r438957156



##########
File path: storm-client/test/jvm/org/apache/storm/executor/ExecutorTransferMultiThreadingTest.java
##########
@@ -126,14 +126,15 @@ public void testExecutorTransfer() throws InterruptedException {
         }
 
         //give producers enough time to insert messages into the queue
-        executorService.awaitTermination(100, TimeUnit.MILLISECONDS);
+        executorService.awaitTermination(1000, TimeUnit.MILLISECONDS);

Review comment:
       This test intentionally submits 10 threads with a 5 thread pool to create race condition between threads. 100ms was just fine in my laptop or VM. On travis, I guess the resource is much limited so the thread contention is more observable. 1s maximum should be good enough. We should be able to lower it but since this value is just a maximum, 1s should be harmless.




----------------------------------------------------------------
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:
users@infra.apache.org