Jackie-Jiang commented on code in PR #20006:
URL: https://github.com/apache/pinot/pull/20006#discussion_r4356818413
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -225,6 +227,7 @@ private static void
validateEffectiveTableConfig(TableConfig tableConfig, Schema
validateInstancePartitionsTypeMapConfig(tableConfig);
validatePartitionedReplicaGroupInstance(tableConfig);
validateInstanceAssignmentConfigs(tableConfig);
+ validateStrictReplicaGroupRouting(tableConfig);
Review Comment:
Deliberate. The check enforces the placement that strict replica-group
routing depends on, for every table using it as well as for upsert and dedup
tables, so it runs on create and update regardless of the skipped validation
types and is not grandfathered: a table that fails it routes queries by
partition while placing segments by something else. The description now calls
out that only `ALL` bypasses it.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -1569,6 +1571,103 @@ private static void
validatePartitionColumnInSegmentPartitionConfig(TableConfig
partitionColumn);
}
+ /// Validates that a table using strict replica-group routing has a segment
assignment that places all segments of
+ /// a partition on exactly one instance per replica-group. Strict
replica-group routing routes all segments with the
+ /// same instances in the ideal state to the same instance, which only
provides partition level consistency when the
+ /// segment assignment keeps each partition on one instance per
replica-group. Upsert and dedup tables rely on the
+ /// same property and are validated regardless of the configured instance
selector.
+ ///
+ /// - OFFLINE: the OFFLINE instance assignment must be replica-group based
with `numInstancesPerPartition` of 1, and
+ /// the OFFLINE segment assignment strategy, if configured, must be
`replicagroup`, since the other strategies
+ /// ignore the partition. The partition column of a partitioned instance
assignment is validated by
+ /// [#validatePartitionedSegmentAssignment]. The instance assignment can
also come from the deprecated
+ /// `replicaGroupStrategyConfig`, which takes effect only together with
the `replicagroup` strategy.
+ /// - REALTIME: CONSUMING segments are placed by their stream partition,
which keeps each partition on one instance
+ /// per replica-group when the CONSUMING instance partitions have a single
partition or one instance per
+ /// partition. COMPLETED segments must not be relocated, which would split
each partition between its CONSUMING
+ /// and COMPLETED instances.
+ ///
+ /// The layout of pre-configured instance partitions lives in ZK and is not
visible here, so the controller validates
+ /// it on the fetched instance partitions instead. Dimension tables are
exempt because every server hosts every
+ /// segment.
+ @VisibleForTesting
+ static void validateStrictReplicaGroupRouting(TableConfig tableConfig) {
+ if (!usesStrictReplicaGroupRouting(tableConfig)) {
+ return;
+ }
+ if (tableConfig.getTableType() == TableType.OFFLINE) {
+ validateStrictReplicaGroupOfflineInstanceAssignment(tableConfig);
+ } else {
+ validateStrictReplicaGroupRealtimeInstanceAssignment(tableConfig);
+ }
+ }
+
+ /// Returns `true` if the segment assignment of the table must place all
segments of a partition on exactly one
+ /// instance per replica-group: the table uses strict replica-group routing,
or is an upsert/dedup table, whose
+ /// allowed instance selectors all rely on this property. Dimension tables
are excluded because every server hosts
+ /// every segment.
+ public static boolean usesStrictReplicaGroupRouting(TableConfig tableConfig)
{
+ if (tableConfig.isDimTable()) {
+ return false;
+ }
+ if (tableConfig.isUpsertEnabled() || tableConfig.isDedupEnabled()) {
+ return true;
+ }
+ RoutingConfig routingConfig = tableConfig.getRoutingConfig();
+ String instanceSelectorType = routingConfig != null ?
routingConfig.getInstanceSelectorType() : null;
+ return
RoutingConfig.STRICT_REPLICA_GROUP_INSTANCE_SELECTOR_TYPE.equalsIgnoreCase(instanceSelectorType);
+ }
+
+ /// Validates the OFFLINE instance assignment config for strict
replica-group routing.
+ private static void
validateStrictReplicaGroupOfflineInstanceAssignment(TableConfig tableConfig) {
+ SegmentAssignmentConfig segmentAssignmentConfig =
+
MapUtils.emptyIfNull(tableConfig.getSegmentAssignmentConfigMap()).get(InstancePartitionsType.OFFLINE.name());
+ String assignmentStrategy =
+ segmentAssignmentConfig != null ?
segmentAssignmentConfig.getAssignmentStrategy() : null;
+ Preconditions.checkState(assignmentStrategy == null
+ ||
AssignmentStrategy.REPLICA_GROUP_SEGMENT_ASSIGNMENT_STRATEGY.equalsIgnoreCase(assignmentStrategy),
+ "Table using strict replica-group routing can only use '%s' OFFLINE
segment assignment strategy, found: %s",
+ AssignmentStrategy.REPLICA_GROUP_SEGMENT_ASSIGNMENT_STRATEGY,
assignmentStrategy);
+ if
(InstancePartitionsUtils.hasPreConfiguredInstancePartitions(tableConfig,
InstancePartitionsType.OFFLINE)) {
+ return;
+ }
+ // The instance assignment config comes from instanceAssignmentConfigMap,
or from the deprecated
+ // replicaGroupStrategyConfig when the replica-group segment assignment
strategy is configured
+ Preconditions.checkState(
+ InstanceAssignmentConfigUtils.allowInstanceAssignment(tableConfig,
InstancePartitionsType.OFFLINE),
+ "Table using strict replica-group routing must have OFFLINE instance
assignment config "
+ + "(instanceAssignmentConfigMap) to ensure all segments of a
partition are assigned to the same server");
+ InstanceReplicaGroupPartitionConfig replicaGroupPartitionConfig =
+ InstanceAssignmentConfigUtils.getInstanceAssignmentConfig(tableConfig,
InstancePartitionsType.OFFLINE)
+ .getReplicaGroupPartitionConfig();
+ Preconditions.checkState(replicaGroupPartitionConfig.isReplicaGroupBased(),
+ "Table using strict replica-group routing must use replica-group based
OFFLINE instance assignment to ensure "
+ + "all segments of a partition are assigned to the same server");
+
Preconditions.checkState(replicaGroupPartitionConfig.getNumInstancesPerPartition()
== 1,
Review Comment:
Fixed. The check now uses the effective count, `numInstancesPerPartition` or
`numInstancesPerReplicaGroup` when it is not set, so
`ReplicaGroupStrategyConfig(null, 1)` passes. When neither is set the count
depends on the instances, and the controller validates the computed layout.
Added a test for the legacy case.
--
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]
