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_r307191364
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/TableRebalancer.java
##########
@@ -212,39 +218,132 @@ 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) {
+
+ LOGGER.info("Will update instance map of segment: {}", segmentId);
+
+ // dump additional detailed info for debugging
+ if (LOGGER.isDebugEnabled()) {
+ // check the debug level beforehand and write everything at once
+ // else we will have to check repeatedly in a loop=
+ StringBuilder sb = new StringBuilder("Current segment hosts and
states:\n");
+ for (Map.Entry<String, String> entry :
currentIdealStateSegmentHosts.entrySet()) {
+ sb.append("HOST: ").append(entry.getKey()).append("STATE:
").append(entry.getValue()).append("\n");
+ }
+ LOGGER.debug(sb.toString());
+ sb = new StringBuilder("Target segment hosts and states:\n");
+ for (Map.Entry<String, String> entry :
targetIdealStateSegmentHosts.entrySet()) {
+ sb.append("HOST: ").append(entry.getKey()).append("STATE:
").append(entry.getValue()).append("\n");
+ }
+ LOGGER.debug(sb.toString());
+ }
- 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);
+ LOGGER.debug("Downtime mode is enabled. Will set to target state at one
go");
+ setTargetState(idealStateToUpdate, segmentId,
targetIdealStateSegmentHosts);
+ ++_rebalancerStats.directTransitions;
return;
}
- MapDifference difference = Maps.difference(targetMap, srcMap);
- if (!difference.entriesInCommon().isEmpty()) {
- // if there are entries in common, there won't be downtime
- LOGGER.debug("Segment " + segmentId + " has common entries between
current and expected ideal state");
- setTargetState(idealState, segmentId, targetMap);
+ // 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);
+ LOGGER.debug("No downtime mode is enabled. Need to keep {} serving
replicas up while rebalancing",
+ minReplicasToKeepUp);
+
+ if (minReplicasToKeepUp >= currentIdealStateSegmentHosts.size()) {
+ // if the minimum number of serving replicas in rebalance config is
+ // greater than or equal to number of replicas of a segment, then it
+ // is impossible to honor the request. so we use the default number
+ // of minimum serving replicas we will keep for no downtime mode
+ // (currently 1)
+ minReplicasToKeepUp =
RebalanceUserConfigConstants.DEFAULT_MIN_REPLICAS_TO_KEEPUP_FOR_NODOWNTIME;
+ }
+ MapDifference difference = Maps.difference(targetIdealStateSegmentHosts,
currentIdealStateSegmentHosts);
+ if (difference.entriesInCommon().size() >= minReplicasToKeepUp) {
+ // if there are enough hosts in common between current and target ideal
states
+ // to satisfy the min replicas condition, then there won't be any
downtime
+ // and we can directly set the current ideal state to target ideal state
+ LOGGER.debug("Current and target ideal states have common hosts. Will
set to target state at one go");
+ setTargetState(idealStateToUpdate, segmentId,
targetIdealStateSegmentHosts);
+ ++_rebalancerStats.directTransitions;
} else {
// remove one entry
-
idealState.getInstanceStateMap(segmentId).remove(srcMap.keySet().stream().findFirst().get());
+ String hostToRemove = "";
+ for (String host : currentIdealStateSegmentHosts.keySet()) {
+ // the common host between current and target ideal
+ // states should be ignored
+ if (!targetIdealStateSegmentHosts.containsKey(host)) {
+ hostToRemove = host;
+ break;
+ }
+ }
+
+ if (!hostToRemove.equals("")) {
+ idealStateToUpdate.getInstanceStateMap(segmentId).remove(hostToRemove);
+ LOGGER.info("Removing host: {} for segment: {}", hostToRemove,
segmentId);
Review comment:
thought could be helpful for debugging -- this method has 3 debug level
messages , one for entry in method and one each for adding and removing a host
----------------------------------------------------------------
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]
