remove RebalanceConfigConstants

pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java

@@ -33,7 +33,6 @@
 import org.apache.pinot.spi.env.PinotConfiguration;
 import org.apache.pinot.spi.filesystem.LocalPinotFS;
 import org.apache.pinot.spi.utils.CommonConstants;
-import org.apache.pinot.spi.utils.RebalanceConfigConstants;
 import org.apache.pinot.spi.utils.TimeUtils;
 
 import static org.apache.pinot.spi.utils.CommonConstants.Controller.CONFIG_OF_CONTROLLER_METRICS_PREFIX;
@@ -961,13 +960,12 @@ public boolean enableSegmentRelocatorLocalTierMigration() {
   }
 
   public long getSegmentRelocatorExternalViewCheckIntervalInMs() {
-    return getProperty(ControllerPeriodicTasksConf.SEGMENT_RELOCATOR_EXTERNAL_VIEW_CHECK_INTERVAL_IN_MS,
-        RebalanceConfigConstants.DEFAULT_EXTERNAL_VIEW_CHECK_INTERVAL_IN_MS);
+    return getProperty(ControllerPeriodicTasksConf.SEGMENT_RELOCATOR_EXTERNAL_VIEW_CHECK_INTERVAL_IN_MS, 1000L);
   }
 
   public long getSegmentRelocatorExternalViewStabilizationTimeoutInMs() {
     return getProperty(ControllerPeriodicTasksConf.SEGMENT_RELOCATOR_EXTERNAL_VIEW_STABILIZATION_TIMEOUT_IN_MS,
-        RebalanceConfigConstants.DEFAULT_EXTERNAL_VIEW_STABILIZATION_TIMEOUT_IN_MS);
+        3600000L);
   }
 
   public boolean isSegmentRelocatorRebalanceTablesSequentially() {

pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTenantRestletResource.java

@@ -584,9 +584,9 @@ public SuccessResponse deleteTenant(
   @ApiOperation(value = "Rebalances all the tables that are part of the tenant")
   public TenantRebalanceResult rebalance(
       @ApiParam(value = "Name of the tenant whose table are to be rebalanced", required = true)
-      @PathParam("tenantName") String tenantName, @ApiParam(required = true) TenantRebalanceConfig context) {
-    context.setTenantName(tenantName);
-    return _tenantRebalancer.rebalance(context);
+      @PathParam("tenantName") String tenantName, @ApiParam(required = true) TenantRebalanceConfig config) {
+    config.setTenantName(tenantName);
+    return _tenantRebalancer.rebalance(config);
   }
 
   @GET

pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/segment/OfflineSegmentAssignment.java

@@ -23,7 +23,6 @@
 import java.util.Map;
 import javax.annotation.Nullable;
 import org.apache.commons.collections.CollectionUtils;
-import org.apache.commons.configuration.Configuration;
 import org.apache.commons.lang3.tuple.Pair;
 import org.apache.pinot.common.assignment.InstancePartitions;
 import org.apache.pinot.common.tier.Tier;
@@ -58,15 +57,6 @@ public List<String> assignSegment(String segmentName, Map<String, Map<String, St
     return instancesAssigned;
   }
 
-  @Deprecated
-  @Override
-  public Map<String, Map<String, String>> rebalanceTable(Map<String, Map<String, String>> currentAssignment,
-      Map<InstancePartitionsType, InstancePartitions> instancePartitionsMap, @Nullable List<Tier> sortedTiers,
-      @Nullable Map<String, InstancePartitions> tierInstancePartitionsMap, Configuration config) {
-    return rebalanceTable(currentAssignment, instancePartitionsMap, sortedTiers, tierInstancePartitionsMap,
-        RebalanceConfig.fromConfiguration(config));
-  }
-
   @Override
   public Map<String, Map<String, String>> rebalanceTable(Map<String, Map<String, String>> currentAssignment,
       Map<InstancePartitionsType, InstancePartitions> instancePartitionsMap, @Nullable List<Tier> sortedTiers,

pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/segment/RealtimeSegmentAssignment.java

@@ -25,7 +25,6 @@
 import java.util.TreeMap;
 import javax.annotation.Nullable;
 import org.apache.commons.collections.CollectionUtils;
-import org.apache.commons.configuration.Configuration;
 import org.apache.commons.lang3.tuple.Pair;
 import org.apache.pinot.common.assignment.InstancePartitions;
 import org.apache.pinot.common.tier.Tier;
@@ -171,15 +170,6 @@ protected List<String> assignConsumingSegment(int segmentPartitionId, InstancePa
     }
   }
 
-  @Deprecated
-  @Override
-  public Map<String, Map<String, String>> rebalanceTable(Map<String, Map<String, String>> currentAssignment,
-      Map<InstancePartitionsType, InstancePartitions> instancePartitionsMap, @Nullable List<Tier> sortedTiers,
-      @Nullable Map<String, InstancePartitions> tierInstancePartitionsMap, Configuration config) {
-    return rebalanceTable(currentAssignment, instancePartitionsMap, sortedTiers, tierInstancePartitionsMap,
-        RebalanceConfig.fromConfiguration(config));
-  }
-
   @Override
   public Map<String, Map<String, String>> rebalanceTable(Map<String, Map<String, String>> currentAssignment,
       Map<InstancePartitionsType, InstancePartitions> instancePartitionsMap, @Nullable List<Tier> sortedTiers,

pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/segment/SegmentAssignment.java

@@ -18,11 +18,9 @@
  */
 package org.apache.pinot.controller.helix.core.assignment.segment;
 
-import java.util.Collections;
 import java.util.List;
 import java.util.Map;
 import javax.annotation.Nullable;
-import org.apache.commons.configuration.Configuration;
 import org.apache.helix.HelixManager;
 import org.apache.pinot.common.assignment.InstancePartitions;
 import org.apache.pinot.common.metrics.ControllerMetrics;
@@ -65,17 +63,7 @@ List<String> assignSegment(String segmentName, Map<String, Map<String, String>>
    * @param config Configuration for the rebalance
    * @return Rebalanced assignment for the segments
    */
-  @Deprecated
   Map<String, Map<String, String>> rebalanceTable(Map<String, Map<String, String>> currentAssignment,
       Map<InstancePartitionsType, InstancePartitions> instancePartitionsMap, @Nullable List<Tier> sortedTiers,
-      @Nullable Map<String, InstancePartitions> tierInstancePartitionsMap, Configuration config);
-
-  /**
-   * Same as above but the use of specific class for rebalance configs.
-   */
-  default Map<String, Map<String, String>> rebalanceTable(Map<String, Map<String, String>> currentAssignment,
-      Map<InstancePartitionsType, InstancePartitions> instancePartitionsMap, @Nullable List<Tier> sortedTiers,
-      @Nullable Map<String, InstancePartitions> tierInstancePartitionsMap, RebalanceConfig config) {
-    return Collections.emptyMap();
-  }
+      @Nullable Map<String, InstancePartitions> tierInstancePartitionsMap, RebalanceConfig config);
 }

pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/RebalanceConfig.java

@@ -21,40 +21,61 @@
 import com.fasterxml.jackson.annotation.JsonProperty;
 import io.swagger.annotations.ApiModel;
 import io.swagger.annotations.ApiModelProperty;
-import org.apache.commons.configuration.Configuration;
-import org.apache.pinot.spi.utils.CommonConstants;
-import org.apache.pinot.spi.utils.RebalanceConfigConstants;
 
 
 @ApiModel
 public class RebalanceConfig {
+  // Whether to rebalance table in dry-run mode
   @JsonProperty("dryRun")
   @ApiModelProperty(example = "false")
   private boolean _dryRun = false;
+
+  // Whether to reassign instances before reassigning segments
   @JsonProperty("reassignInstances")
   @ApiModelProperty(example = "false")
   private boolean _reassignInstances = false;
+
+  // Whether to reassign CONSUMING segments
   @JsonProperty("includeConsuming")
   @ApiModelProperty(example = "false")
   private boolean _includeConsuming = false;
+
+  // Whether to rebalance table in bootstrap mode (regardless of minimum segment movement, reassign all segments in a
+  // round-robin fashion as if adding new segments to an empty table)
   @JsonProperty("bootstrap")
   @ApiModelProperty(example = "false")
   private boolean _bootstrap = false;
+
+  // Whether to allow downtime for the rebalance
   @JsonProperty("downtime")
   @ApiModelProperty(example = "false")
   private boolean _downtime = false;
+
+  // For no-downtime rebalance, minimum number of replicas to keep alive during rebalance, or maximum number of replicas
+  // allowed to be unavailable if value is negative
   @JsonProperty("minAvailableReplicas")
   @ApiModelProperty(example = "1")
   private int _minAvailableReplicas = 1;
+
+  // Whether to use best-efforts to rebalance (not fail the rebalance when the no-downtime contract cannot be achieved)
+  // When using best-efforts to rebalance, the following scenarios won't fail the rebalance (will log warnings instead):
+  // - Segment falls into ERROR state in ExternalView -> count ERROR state as good state
+  // - ExternalView has not converged within the maximum wait time -> continue to the next stage
   @JsonProperty("bestEfforts")
   @ApiModelProperty(example = "false")
   private boolean _bestEfforts = false;
+
+  // The check on external view can be very costly when the table has very large ideal and external states, i.e. when
+  // having a huge number of segments. These two configs help reduce the cpu load on controllers, e.g. by doing the
+  // check less frequently and bail out sooner to rebalance at best effort if configured so.
   @JsonProperty("externalViewCheckIntervalInMs")
   @ApiModelProperty(example = "1000")
   private long _externalViewCheckIntervalInMs = 1000L;
+
   @JsonProperty("externalViewStabilizationTimeoutInMs")
   @ApiModelProperty(example = "3600000")
   private long _externalViewStabilizationTimeoutInMs = 3600000L;
+
   @JsonProperty("updateTargetTier")
   @ApiModelProperty(example = "false")
   private boolean _updateTargetTier = false;
@@ -165,29 +186,4 @@ public static RebalanceConfig copy(RebalanceConfig cfg) {
     rc._jobId = cfg._jobId;
     return rc;
   }
-
-  // Helper method to help deprecate the use of Configuration to keep rebalance configs.
-  public static RebalanceConfig fromConfiguration(Configuration cfg) {
-    RebalanceConfig rc = new RebalanceConfig();
-    rc.setDryRun(cfg.getBoolean(RebalanceConfigConstants.DRY_RUN, RebalanceConfigConstants.DEFAULT_DRY_RUN));
-    rc.setReassignInstances(cfg.getBoolean(RebalanceConfigConstants.REASSIGN_INSTANCES,
-        RebalanceConfigConstants.DEFAULT_REASSIGN_INSTANCES));
-    rc.setIncludeConsuming(
-        cfg.getBoolean(RebalanceConfigConstants.INCLUDE_CONSUMING, RebalanceConfigConstants.DEFAULT_INCLUDE_CONSUMING));
-    rc.setBootstrap(cfg.getBoolean(RebalanceConfigConstants.BOOTSTRAP, RebalanceConfigConstants.DEFAULT_BOOTSTRAP));
-    rc.setDowntime(cfg.getBoolean(RebalanceConfigConstants.DOWNTIME, RebalanceConfigConstants.DEFAULT_DOWNTIME));
-    rc.setMinAvailableReplicas(cfg.getInt(RebalanceConfigConstants.MIN_REPLICAS_TO_KEEP_UP_FOR_NO_DOWNTIME,
-        RebalanceConfigConstants.DEFAULT_MIN_REPLICAS_TO_KEEP_UP_FOR_NO_DOWNTIME));
-    rc.setBestEfforts(
-        cfg.getBoolean(RebalanceConfigConstants.BEST_EFFORTS, RebalanceConfigConstants.DEFAULT_BEST_EFFORTS));
-    rc.setExternalViewCheckIntervalInMs(cfg.getLong(RebalanceConfigConstants.EXTERNAL_VIEW_CHECK_INTERVAL_IN_MS,
-        RebalanceConfigConstants.DEFAULT_EXTERNAL_VIEW_CHECK_INTERVAL_IN_MS));
-    rc.setExternalViewStabilizationTimeoutInMs(
-        cfg.getLong(RebalanceConfigConstants.EXTERNAL_VIEW_STABILIZATION_TIMEOUT_IN_MS,
-            RebalanceConfigConstants.DEFAULT_EXTERNAL_VIEW_STABILIZATION_TIMEOUT_IN_MS));
-    rc.setUpdateTargetTier(cfg.getBoolean(RebalanceConfigConstants.UPDATE_TARGET_TIER,
-        RebalanceConfigConstants.DEFAULT_UPDATE_TARGET_TIER));
-    rc.setJobId(cfg.getString(CommonConstants.ControllerJob.JOB_ID, null));
-    return rc;
-  }
 }

pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancer.java

@@ -33,7 +33,6 @@
 import java.util.function.ToIntFunction;
 import javax.annotation.Nullable;
 import org.apache.commons.collections.CollectionUtils;
-import org.apache.commons.configuration.Configuration;
 import org.apache.commons.lang3.tuple.Pair;
 import org.apache.commons.lang3.tuple.Triple;
 import org.apache.helix.AccessOption;
@@ -137,11 +136,6 @@ public static String createUniqueRebalanceJobIdentifier() {
     return UUID.randomUUID().toString();
   }
 
-  @Deprecated
-  public RebalanceResult rebalance(TableConfig tableConfig, Configuration rebalanceConfig) {
-    return rebalance(tableConfig, RebalanceConfig.fromConfiguration(rebalanceConfig));
-  }
-
   public RebalanceResult rebalance(TableConfig tableConfig, RebalanceConfig rebalanceConfig) {
     long startTimeMs = System.currentTimeMillis();
     String tableNameWithType = tableConfig.getTableName();

pinot-spi/src/main/java/org/apache/pinot/spi/utils/builder/ControllerRequestURLBuilder.java

@@ -28,7 +28,6 @@
 import org.apache.commons.lang3.StringUtils;
 import org.apache.pinot.spi.config.table.TableType;
 import org.apache.pinot.spi.config.table.assignment.InstancePartitionsType;
-import org.apache.pinot.spi.utils.RebalanceConfigConstants;
 import org.apache.pinot.spi.utils.StringUtil;
 
 
@@ -199,29 +198,26 @@ public String forUpdateTableConfig(String tableName) {
   }
 
   public String forTableRebalance(String tableName, String tableType) {
-    return forTableRebalance(tableName, tableType, RebalanceConfigConstants.DEFAULT_DRY_RUN,
-        RebalanceConfigConstants.DEFAULT_REASSIGN_INSTANCES, RebalanceConfigConstants.DEFAULT_INCLUDE_CONSUMING,
-        RebalanceConfigConstants.DEFAULT_DOWNTIME,
-        RebalanceConfigConstants.DEFAULT_MIN_REPLICAS_TO_KEEP_UP_FOR_NO_DOWNTIME);
+    return forTableRebalance(tableName, tableType, false, false, false, false, 1);
   }
 
   public String forTableRebalance(String tableName, String tableType, boolean dryRun, boolean reassignInstances,
       boolean includeConsuming, boolean downtime, int minAvailableReplicas) {
     StringBuilder stringBuilder =
         new StringBuilder(StringUtil.join("/", _baseUrl, "tables", tableName, "rebalance?type=" + tableType));
-    if (dryRun != RebalanceConfigConstants.DEFAULT_DRY_RUN) {
+    if (dryRun) {
       stringBuilder.append("&dryRun=").append(dryRun);
     }
-    if (reassignInstances != RebalanceConfigConstants.DEFAULT_REASSIGN_INSTANCES) {
+    if (reassignInstances) {
       stringBuilder.append("&reassignInstances=").append(reassignInstances);
     }
-    if (includeConsuming != RebalanceConfigConstants.DEFAULT_INCLUDE_CONSUMING) {
+    if (includeConsuming) {
       stringBuilder.append("&includeConsuming=").append(includeConsuming);
     }
-    if (downtime != RebalanceConfigConstants.DEFAULT_DOWNTIME) {
+    if (downtime) {
       stringBuilder.append("&downtime=").append(downtime);
     }
-    if (minAvailableReplicas != RebalanceConfigConstants.DEFAULT_MIN_REPLICAS_TO_KEEP_UP_FOR_NO_DOWNTIME) {
+    if (minAvailableReplicas != 1) {
       stringBuilder.append("&minAvailableReplicas=").append(minAvailableReplicas);
     }
     return stringBuilder.toString();

pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/RebalanceTableCommand.java

@@ -20,7 +20,6 @@
 
 import org.apache.pinot.controller.helix.core.rebalance.RebalanceResult;
 import org.apache.pinot.spi.utils.JsonUtils;
-import org.apache.pinot.spi.utils.RebalanceConfigConstants;
 import org.apache.pinot.tools.Command;
 import org.apache.pinot.tools.PinotTableRebalancer;
 import org.slf4j.Logger;
@@ -80,12 +79,11 @@ public class RebalanceTableCommand extends AbstractBaseAdminCommand implements C
 
   @CommandLine.Option(names = {"-externalViewCheckIntervalInMs"},
       description = "How often to check if external view converges with ideal view")
-  private long _externalViewCheckIntervalInMs = RebalanceConfigConstants.DEFAULT_EXTERNAL_VIEW_CHECK_INTERVAL_IN_MS;
+  private long _externalViewCheckIntervalInMs = 1000L;
 
   @CommandLine.Option(names = {"-externalViewStabilizationTimeoutInMs"},
       description = "How long to wait till external view converges with ideal view")
-  private long _externalViewStabilizationTimeoutInMs =
-      RebalanceConfigConstants.DEFAULT_EXTERNAL_VIEW_STABILIZATION_TIMEOUT_IN_MS;
+  private long _externalViewStabilizationTimeoutInMs = 3600000L;
 
   @CommandLine.Option(names = {"-help", "-h", "--h", "--help"}, help = true, description = "Print this message")
   private boolean _help = false;