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



##########
File path: 
samza-core/src/main/java/org/apache/samza/clustermanager/StandbyContainerManager.java
##########
@@ -361,8 +407,41 @@ private FailoverMetadata 
registerActiveContainerFailure(String activeContainerID
   }
 
   /**
-   * Check if matching this SamzaResourceRequest to the given resource, meets 
all standby-container container constraints.
+   * This method checks from the config if standby allocation is fault domain 
aware or not, and requests resources accordingly.
+   *
+   * @param containerAllocator ContainerAllocator object that requests for 
resources from the resource manager
+   * @param containerID Samza container ID that will be run when a resource is 
allocated for this request
+   * @param preferredHost name of the host that you prefer to run the 
processor on
+   */
+  void checkFaultDomainAwarenessEnabledAndRequestResource(ContainerAllocator 
containerAllocator, String containerID, String preferredHost) {

Review comment:
       nit: I'd prefer to rename the method to `requestResource` since the 
intent of the method that way is clear. i.e. only request resource and 
potentially return the `SamzaResourceRequest`.
   
   What and how it does to request resource is kept within and can be inferred 
by reading the method implementation. The name seems too long and the fact that 
this returns void makes it unusable in some places which has the exact boiler 
plate code.




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


Reply via email to