You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@samza.apache.org by GitBox <gi...@apache.org> on 2020/06/09 16:09:26 UTC
[GitHub] [samza] Sanil15 opened a new pull request #1378: Adding cleanup for Container Placement tests using threads, coordinator stream stores
Sanil15 opened a new pull request #1378:
URL: https://github.com/apache/samza/pull/1378
**Improvement** [Bug fix]: Adding teardown / cleanup to Container placements integration tests
**API changes:** None
**Symptoms:**: Creates OOMs when test suite is executed
```
Exception: java.lang.OutOfMemoryError thrown from the UncaughtExceptionHandler in thread "RMI RenewClean-[kwu-mn1.linkedin.biz:52577]"
Exception: java.lang.OutOfMemoryError thrown from the UncaughtExceptionHandler in thread "Container Allocator Thread"
Exception: java.lang.OutOfMemoryError thrown from the UncaughtExceptionHandler in thread "RMI RenewClean-[kwu-mn1.linkedin.biz:52581]"
Exception: java.lang.OutOfMemoryError thrown from the UncaughtExceptionHandler in thread "ContainerPlacement Request Allocator Thread"
Exception: java.lang.OutOfMemoryError thrown from the UncaughtExceptionHandler in thread "Container Allocator Thread"
Exception: java.lang.OutOfMemoryError thrown from the UncaughtExceptionHandler in thread "Container Allocator Thread"
Exception: java.lang.OutOfMemoryError thrown from the UncaughtExceptionHandler in thread "Container Allocator Thread"
```
**Upgrade Instructions:** None
**Usage Instructions:** None
----------------------------------------------------------------
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] [samza] asfgit closed pull request #1378: SAMZA-2544: Adding cleanup for Container Placement tests using threads, coordinator stream stores
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #1378:
URL: https://github.com/apache/samza/pull/1378
----------------------------------------------------------------
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] [samza] mynameborat commented on a change in pull request #1378: SAMZA-2544: Adding cleanup for Container Placement tests using threads, coordinator stream stores
Posted by GitBox <gi...@apache.org>.
mynameborat commented on a change in pull request #1378:
URL: https://github.com/apache/samza/pull/1378#discussion_r437003992
##########
File path: samza-core/src/test/java/org/apache/samza/clustermanager/TestContainerPlacementActions.java
##########
@@ -1011,4 +1026,13 @@ private void assertBadRequests(String processorId, String destinationHost, Conta
// Request shall be deleted as soon as it is acted upon
assertFalse(containerPlacementMetadataStore.readContainerPlacementRequestMessage(requestMessage.getUuid()).isPresent());
}
+
+ private void cleanUpRequestAllocatorThread(ContainerPlacementRequestAllocator requestAllocator, Thread containerPlacementRequestAllocatorThread) {
+ requestAllocator.stop();
+ try {
+ containerPlacementRequestAllocatorThread.join();
+ } catch (InterruptedException ie) {
+ Thread.currentThread().interrupt();
+ }
+ }
Review comment:
Can this be part of `AfterMethod` tearDown provided you extract the request allocator to class level? It still gets cleaned up for tests that forget to invoke this explicitly.
##########
File path: samza-core/src/test/java/org/apache/samza/clustermanager/TestContainerPlacementActions.java
##########
@@ -266,9 +274,10 @@ public Void answer(InvocationOnMock invocation) {
@Test(timeout = 30000)
public void testActionQueuingForConsecutivePlacementActions() throws Exception {
// Spawn a Request Allocator Thread
- Thread requestAllocatorThread = new Thread(
- new ContainerPlacementRequestAllocator(containerPlacementMetadataStore, cpm, new ApplicationConfig(config)),
- "ContainerPlacement Request Allocator Thread");
+ ContainerPlacementRequestAllocator requestAllocator =
+ new ContainerPlacementRequestAllocator(containerPlacementMetadataStore, cpm, new ApplicationConfig(config));
+ Thread requestAllocatorThread = new Thread(requestAllocator, "ContainerPlacement Request Allocator Thread");
+
Review comment:
Can this be extracted at the class level and initialize before every tests?
----------------------------------------------------------------
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] [samza] Sanil15 commented on pull request #1378: SAMZA-2544: Adding cleanup for Container Placement tests using threads, coordinator stream stores
Posted by GitBox <gi...@apache.org>.
Sanil15 commented on pull request #1378:
URL: https://github.com/apache/samza/pull/1378#issuecomment-640879131
----------------------------------------------------------------
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] [samza] Sanil15 commented on a change in pull request #1378: SAMZA-2544: Adding cleanup for Container Placement tests using threads, coordinator stream stores
Posted by GitBox <gi...@apache.org>.
Sanil15 commented on a change in pull request #1378:
URL: https://github.com/apache/samza/pull/1378#discussion_r437009365
##########
File path: samza-core/src/test/java/org/apache/samza/clustermanager/TestContainerPlacementActions.java
##########
@@ -1011,4 +1026,13 @@ private void assertBadRequests(String processorId, String destinationHost, Conta
// Request shall be deleted as soon as it is acted upon
assertFalse(containerPlacementMetadataStore.readContainerPlacementRequestMessage(requestMessage.getUuid()).isPresent());
}
+
+ private void cleanUpRequestAllocatorThread(ContainerPlacementRequestAllocator requestAllocator, Thread containerPlacementRequestAllocatorThread) {
+ requestAllocator.stop();
+ try {
+ containerPlacementRequestAllocatorThread.join();
+ } catch (InterruptedException ie) {
+ Thread.currentThread().interrupt();
+ }
+ }
Review comment:
Not every test instantiates a ContainerPlacementRequestAllocator only two tests do hence this cleanup.
Also two tests use this individually, notice its not used in setup
##########
File path: samza-core/src/test/java/org/apache/samza/clustermanager/TestContainerPlacementActions.java
##########
@@ -266,9 +274,10 @@ public Void answer(InvocationOnMock invocation) {
@Test(timeout = 30000)
public void testActionQueuingForConsecutivePlacementActions() throws Exception {
// Spawn a Request Allocator Thread
- Thread requestAllocatorThread = new Thread(
- new ContainerPlacementRequestAllocator(containerPlacementMetadataStore, cpm, new ApplicationConfig(config)),
- "ContainerPlacement Request Allocator Thread");
+ ContainerPlacementRequestAllocator requestAllocator =
+ new ContainerPlacementRequestAllocator(containerPlacementMetadataStore, cpm, new ApplicationConfig(config));
+ Thread requestAllocatorThread = new Thread(requestAllocator, "ContainerPlacement Request Allocator Thread");
+
Review comment:
No this is not used by every tests only two tests use this route of injecting control messages not all the tests
----------------------------------------------------------------
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