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:
[email protected]
