praveenc7 commented on code in PR #16672:
URL: https://github.com/apache/pinot/pull/16672#discussion_r2364874985
##########
pinot-controller/src/main/java/org/apache/pinot/controller/workload/scheme/TablePropagationScheme.java:
##########
@@ -19,60 +19,263 @@
package org.apache.pinot.controller.workload.scheme;
import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
+import javax.annotation.Nullable;
+import org.apache.helix.HelixManager;
+import org.apache.helix.model.ExternalView;
+import org.apache.pinot.common.assignment.InstancePartitionsUtils;
+import org.apache.pinot.common.utils.helix.HelixHelper;
import org.apache.pinot.controller.helix.core.PinotHelixResourceManager;
+import org.apache.pinot.controller.workload.splitter.CostSplitter;
import org.apache.pinot.spi.config.table.TableType;
+import org.apache.pinot.spi.config.table.assignment.InstancePartitionsType;
+import org.apache.pinot.spi.config.workload.InstanceCost;
import org.apache.pinot.spi.config.workload.NodeConfig;
+import org.apache.pinot.spi.config.workload.PropagationEntity;
+import org.apache.pinot.spi.config.workload.PropagationEntityOverrides;
+import org.apache.pinot.spi.utils.CommonConstants;
import org.apache.pinot.spi.utils.builder.TableNameBuilder;
/**
- * TablePropagationScheme is used to resolve instances based on the {@link
NodeConfig} and {@link NodeConfig.Type}
- * It resolves the instances based on the table names specified in the node
configuration
+ * A {@code TablePropagationScheme} resolves Pinot instances based on table
names in a node
+ * configuration.
+ *
*/
public class TablePropagationScheme implements PropagationScheme {
- private static PinotHelixResourceManager _pinotHelixResourceManager;
+ private final PinotHelixResourceManager _pinotHelixResourceManager;
public TablePropagationScheme(PinotHelixResourceManager
pinotHelixResourceManager) {
_pinotHelixResourceManager = pinotHelixResourceManager;
}
- @Override
+ /**
+ * Resolves the union of all instances across all cost splits for the given
node config.
+ *
+ * Example:
+ * <pre>
+ * { "Broker_Instance_1", "Broker_Instance_2", "Server_Instance_1" }
+ * </pre>
+ */
public Set<String> resolveInstances(NodeConfig nodeConfig) {
Set<String> instances = new HashSet<>();
- List<String> tableNames = nodeConfig.getPropagationScheme().getValues();
- Map<String, Map<NodeConfig.Type, Set<String>>> tableWithTypeToHelixTags
- = PropagationUtils.getTableToHelixTags(_pinotHelixResourceManager);
- Map<String, Set<String>> helixTagToInstances
- =
PropagationUtils.getHelixTagToInstances(_pinotHelixResourceManager);
Review Comment:
Moving away from this approach because it doesn’t honor the
InstancePartitionMap, which reflects the actual state of instances assigned to
a table. Relying on helixTag could incorrectly split costs across instances
that aren’t even serving the table.
InstancePartitionMap is accurate in most cases, except during swaps—but we
already handle that by refreshing the workload after . While race conditions
are possible, they should be rare. For the initial version, I’m planning to
rely on InstancePartitionMap itself.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
