[GitHub] [samza] mynameborat commented on a change in pull request #1432: SAMZA-2592: Add support for handling stop failures for container kill commands

[GitBox <gi...@apache.org>]

mynameborat commented on a change in pull request #1432:
URL: https://github.com/apache/samza/pull/1432#discussion_r493583712



##########
File path: samza-core/src/main/java/org/apache/samza/clustermanager/ContainerManager.java
##########
@@ -202,7 +202,21 @@ void handleContainerStop(String processorId, String containerId, String preferre
       LOG.info("Setting the container state with Processor ID: {} to be stopped because of existing ContainerPlacement action: {}",
           processorId, metadata);
       metadata.setContainerStatus(ContainerPlacementMetadata.ContainerStatus.STOPPED);
-    } else if (standbyContainerManager.isPresent()) {
+      /**
+       * In the kill -9 case since container is successfully stopped we report placement action as success and fallback
+       * to normal restart path
+       */
+      if (metadata.getDestinationHost().equals(FORCE_RESTART_LAST_SEEN)) {
+        updateContainerPlacementActionStatus(metadata, ContainerPlacementMessage.StatusCode.SUCCEEDED,
+            String.format("Successfully stopped the container %s on host %s, falling back to normal restart path",
+                processorId, metadata.getSourceHost()));
+      } else {
+        // In this case ContainerManager will handle the next step from Allocator
+        return;
+      }
+    }
+
+    if (standbyContainerManager.isPresent()) {

Review comment:
       why are we changing the flow from `if -> else if` to `if(...) {} if (...)`? It is harder to read this way and also to break out in the middle makes it harder to read in general.
   
   On top of readability with multiple control statements in between, we seem to now change the behavior too. You ideally want either container placement to `handleContainerStop` or standby deal with or delegate to `containerAllocator`. If that is not the case, can you update the PR description to also include what standby bug are you fixing?  
   

##########
File path: samza-core/src/main/java/org/apache/samza/clustermanager/ContainerManager.java
##########
@@ -202,7 +202,21 @@ void handleContainerStop(String processorId, String containerId, String preferre
       LOG.info("Setting the container state with Processor ID: {} to be stopped because of existing ContainerPlacement action: {}",
           processorId, metadata);
       metadata.setContainerStatus(ContainerPlacementMetadata.ContainerStatus.STOPPED);
-    } else if (standbyContainerManager.isPresent()) {
+      /**
+       * In the kill -9 case since container is successfully stopped we report placement action as success and fallback
+       * to normal restart path
+       */
+      if (metadata.getDestinationHost().equals(FORCE_RESTART_LAST_SEEN)) {
+        updateContainerPlacementActionStatus(metadata, ContainerPlacementMessage.StatusCode.SUCCEEDED,
+            String.format("Successfully stopped the container %s on host %s, falling back to normal restart path",
+                processorId, metadata.getSourceHost()));
+      } else {
+        // In this case ContainerManager will handle the next step from Allocator

Review comment:
       is this case referring to  `move` command? if so, can you be explicit?




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