somandal commented on code in PR #15575:
URL: https://github.com/apache/pinot/pull/15575#discussion_r2049749036
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/DefaultRebalancePreChecker.java:
##########
@@ -379,6 +382,58 @@ private RebalancePreCheckerResult
checkRebalanceConfig(RebalanceConfig rebalance
: RebalancePreCheckerResult.warn(StringUtil.join("\n",
warnings.toArray(String[]::new)));
}
+ private RebalancePreCheckerResult checkReplicaGroups(TableConfig
tableConfig, RebalanceConfig rebalanceConfig) {
+ String message;
+ if (tableConfig.getTableType() == TableType.OFFLINE) {
+ message = "OFFLINE segments - " + getReplicaGroupInfo(tableConfig,
InstancePartitionsType.OFFLINE);
+ } else {
+ // for realtime table
+ message = "COMPLETED segments - " + getReplicaGroupInfo(tableConfig,
InstancePartitionsType.COMPLETED) + "\n"
+ + "CONSUMING segments - " + getReplicaGroupInfo(tableConfig,
InstancePartitionsType.CONSUMING);
+ }
+ String tierMessage = "";
+ if (tableConfig.getTierConfigsList() != null) {
+ List<String> tierMessageList = new ArrayList<>();
+ for (TierConfig tierConfig : tableConfig.getTierConfigsList()) {
+ tierMessageList.add(tierConfig.getName() + " tier - " +
getReplicaGroupInfo(tableConfig, tierConfig.getName()));
+ }
+ tierMessage = "\n" + StringUtil.join("\n",
tierMessageList.toArray(String[]::new));
+ }
+ if (rebalanceConfig.isReassignInstances()) {
+ return RebalancePreCheckerResult.pass(message + tierMessage);
+ }
+ return RebalancePreCheckerResult.warn(
Review Comment:
we should only warn if `replicaGroups` is enabled, right? For balanced
assignment, it doesn't matter if we set `reassignInstances` or not, since it
will always update based on the currently tagged list
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/DefaultRebalancePreChecker.java:
##########
@@ -379,6 +382,58 @@ private RebalancePreCheckerResult
checkRebalanceConfig(RebalanceConfig rebalance
: RebalancePreCheckerResult.warn(StringUtil.join("\n",
warnings.toArray(String[]::new)));
}
+ private RebalancePreCheckerResult checkReplicaGroups(TableConfig
tableConfig, RebalanceConfig rebalanceConfig) {
+ String message;
+ if (tableConfig.getTableType() == TableType.OFFLINE) {
+ message = "OFFLINE segments - " + getReplicaGroupInfo(tableConfig,
InstancePartitionsType.OFFLINE);
+ } else {
+ // for realtime table
+ message = "COMPLETED segments - " + getReplicaGroupInfo(tableConfig,
InstancePartitionsType.COMPLETED) + "\n"
+ + "CONSUMING segments - " + getReplicaGroupInfo(tableConfig,
InstancePartitionsType.CONSUMING);
+ }
+ String tierMessage = "";
+ if (tableConfig.getTierConfigsList() != null) {
+ List<String> tierMessageList = new ArrayList<>();
+ for (TierConfig tierConfig : tableConfig.getTierConfigsList()) {
+ tierMessageList.add(tierConfig.getName() + " tier - " +
getReplicaGroupInfo(tableConfig, tierConfig.getName()));
+ }
+ tierMessage = "\n" + StringUtil.join("\n",
tierMessageList.toArray(String[]::new));
+ }
+ if (rebalanceConfig.isReassignInstances()) {
+ return RebalancePreCheckerResult.pass(message + tierMessage);
+ }
+ return RebalancePreCheckerResult.warn(
+ "reassignInstances is disabled, replica groups may not update.\n" +
message + tierMessage);
Review Comment:
nit: reword to "replica groups may not be update"
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/DefaultRebalancePreChecker.java:
##########
@@ -379,6 +382,58 @@ private RebalancePreCheckerResult
checkRebalanceConfig(RebalanceConfig rebalance
: RebalancePreCheckerResult.warn(StringUtil.join("\n",
warnings.toArray(String[]::new)));
}
+ private RebalancePreCheckerResult checkReplicaGroups(TableConfig
tableConfig, RebalanceConfig rebalanceConfig) {
+ String message;
+ if (tableConfig.getTableType() == TableType.OFFLINE) {
+ message = "OFFLINE segments - " + getReplicaGroupInfo(tableConfig,
InstancePartitionsType.OFFLINE);
+ } else {
+ // for realtime table
+ message = "COMPLETED segments - " + getReplicaGroupInfo(tableConfig,
InstancePartitionsType.COMPLETED) + "\n"
+ + "CONSUMING segments - " + getReplicaGroupInfo(tableConfig,
InstancePartitionsType.CONSUMING);
+ }
+ String tierMessage = "";
+ if (tableConfig.getTierConfigsList() != null) {
+ List<String> tierMessageList = new ArrayList<>();
+ for (TierConfig tierConfig : tableConfig.getTierConfigsList()) {
+ tierMessageList.add(tierConfig.getName() + " tier - " +
getReplicaGroupInfo(tableConfig, tierConfig.getName()));
+ }
+ tierMessage = "\n" + StringUtil.join("\n",
tierMessageList.toArray(String[]::new));
+ }
+ if (rebalanceConfig.isReassignInstances()) {
+ return RebalancePreCheckerResult.pass(message + tierMessage);
+ }
+ return RebalancePreCheckerResult.warn(
+ "reassignInstances is disabled, replica groups may not update.\n" +
message + tierMessage);
+ }
+
+ private String getReplicaGroupInfo(TableConfig tableConfig,
InstancePartitionsType type) {
+ if (!InstanceAssignmentConfigUtils.allowInstanceAssignment(tableConfig,
type)) {
+ return "Replica Groups are not enabled, replication: " +
tableConfig.getReplication();
+ }
+ return getReplicaGroupInfo(tableConfig, type.toString());
+ }
+
+ private static String getReplicaGroupInfo(TableConfig tableConfig, String
type) {
+ Map<String, InstanceAssignmentConfig> instanceAssignmentConfigMap =
tableConfig.getInstanceAssignmentConfigMap();
+ if (instanceAssignmentConfigMap != null &&
instanceAssignmentConfigMap.containsKey(type)) {
+ InstanceReplicaGroupPartitionConfig instanceReplicaGroupPartitionConfig =
+
instanceAssignmentConfigMap.get(type).getReplicaGroupPartitionConfig();
+ if (!instanceReplicaGroupPartitionConfig.isReplicaGroupBased()) {
+ return "Replica Groups are not enabled, replication: " +
tableConfig.getReplication();
Review Comment:
Can you check once which `replication` is picked up if the
InstanceAssignmentConfig is present, but in that the replicaGroups is disabled
and `replication` is set up? What if it is disabled but we set
`numReplicaGroups` to some value and also set `replication`? Just want to
ensure the correct one is picked up (good way to test is to try a rebalance
dry-run locally and see which change results in NO_OP vs. showing that
rebalance is needed)
e.g.
```
"instanceAssignmentConfigMap": {
"OFFLINE": {
"tagPoolConfig": {
"tag": "DefaultTenant_OFFLINE",
"poolBased": false,
"numPools": 0
},
"replicaGroupPartitionConfig": {
"replicaGroupBased": false,
"numReplicaGroups": 3,
...
...
},
..
}
},
```
--
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.
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
