siddharthteotia commented on a change in pull request #4446: Add support in the 
rebalancer for the user to provide minimum number of serving replicas
URL: https://github.com/apache/incubator-pinot/pull/4446#discussion_r305029885
 
 

 ##########
 File path: 
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/TableRebalancer.java
 ##########
 @@ -212,39 +214,87 @@ private IdealState getNextState(IdealState currentState, 
IdealState targetState,
   }
 
   /**
-   * Updates a segment mapping if needed. In "downtime" mode or if there are 
common elements between source and
-   * target mapping, the segment mapping is set to the target mapping directly.
-   * In a no-downtime, if there are no commmon elements, one element of the 
source mapping is replaced with one
-   * from the target mapping.
+   * Updates a segment mapping if needed. In "downtime" mode.
+   * the segment mapping is set to the target mapping directly.
+   * In no-downtime mode, one element of the source mapping is replaced
+   * with one from the target mapping. Secondly, with downtime mode
+   * and if there are some common hosts, then we check if the number
+   * of common hosts are enough to satisfy the minimum number
+   * of serving replicas requirement as specified in rebalance
+   * config.
+   *
+   * @param segmentId segment id
+   * @param currentIdealStateSegmentHosts map of this segment's hosts 
(replicas)
+   *                                     in current ideal state
+   * @param targetIdealStateSegmentHosts map of this segment's hosts (replicas)
+   *                                     in target ideal state
+   * @param idealStateToUpdate the ideal state that is updated as (caller
+   *                           passes this as a copy of current ideal state)
+   * @param rebalanceUserConfig rebalance configuration
    */
   @VisibleForTesting
-  public void updateSegmentIfNeeded(String segmentId, Map<String, String> 
srcMap, Map<String, String> targetMap,
-      IdealState idealState, Configuration rebalanceUserConfig) {
+  public void updateSegmentIfNeeded(String segmentId, Map<String, String> 
currentIdealStateSegmentHosts,
+      Map<String, String> targetIdealStateSegmentHosts, IdealState 
idealStateToUpdate,
+      Configuration rebalanceUserConfig) {
 
-    if (srcMap == null) {
+    if (currentIdealStateSegmentHosts == null) {
       //segment can be missing if retention manager has deleted it
       LOGGER.info("Segment " + segmentId + " missing from current idealState. 
Skipping it.");
       return;
     }
 
     if (rebalanceUserConfig
+        // in downtime mode, set the current ideal state to target at one go
         .getBoolean(RebalanceUserConfigConstants.DOWNTIME, 
RebalanceUserConfigConstants.DEFAULT_DOWNTIME)) {
-      setTargetState(idealState, segmentId, targetMap);
+      setTargetState(idealStateToUpdate, segmentId, 
targetIdealStateSegmentHosts);
       return;
     }
 
-    MapDifference difference = Maps.difference(targetMap, srcMap);
-    if (!difference.entriesInCommon().isEmpty()) {
-      // if there are entries in common, there won't be downtime
+    // we are in no-downtime mode
+    int minReplicasToKeepUp = 
rebalanceUserConfig.getInt(RebalanceUserConfigConstants.MIN_REPLICAS_TO_KEEPUP_FOR_NODOWNTIME,
+        
RebalanceUserConfigConstants.DEFAULT_MIN_REPLICAS_TO_KEEPUP_FOR_NODOWNTIME);
+    if (minReplicasToKeepUp >= currentIdealStateSegmentHosts.size()) {
+      // if the minimum number of serving replicas in rebalance config is
 
 Review comment:
   One thought here was that PinotTableRestletResource can first use an API of 
TableRebalancer to check if the user specified min replica number makes sense 
or not. TableRebalancer will look at ideal state and see the number of replicas 
for each segment. If the user specified value >= number of replicas then we 
throw an exception right there before rest api code submits the async rebalance 
task. 
   
   Right now such an API is not implemented. Rather we take the user specified 
value as is and at the time of rebalancing if we found the value to be invalid, 
we default to 1

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


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to