Jackie-Jiang commented on code in PR #20006:
URL: https://github.com/apache/pinot/pull/20006#discussion_r4362709072
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -1569,6 +1571,116 @@ 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 one instance per partition, and the
+ /// OFFLINE segment assignment strategy, if configured, must be partition
based (`replicagroup` or
+ /// `roundrobinreplicagroup`), 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 (!requiresStrictReplicaGroupAssignment(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 requiresStrictReplicaGroupAssignment(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.PARTITION_BASED_STRATEGIES.contains(assignmentStrategy.toLowerCase(Locale.ROOT)),
+ "Table using strict replica-group routing can only use '%s' or '%s'
OFFLINE segment assignment strategy, "
+ + "found: %s",
AssignmentStrategy.REPLICA_GROUP_SEGMENT_ASSIGNMENT_STRATEGY,
+
AssignmentStrategy.ROUND_ROBIN_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(getNumInstancesPerPartition(replicaGroupPartitionConfig)
<= 1,
+ "Table using strict replica-group routing must have one instance per
partition (numInstancesPerPartition, or "
+ + "numInstancesPerReplicaGroup when it is not set) in OFFLINE
instance assignment config to ensure all "
+ + "segments of a partition are assigned to the same server");
+ }
+
+ /// Validates the REALTIME instance assignment config for strict
replica-group routing.
+ private static void
validateStrictReplicaGroupRealtimeInstanceAssignment(TableConfig tableConfig) {
+
Preconditions.checkState(!InstanceAssignmentConfigUtils.shouldRelocateCompletedSegments(tableConfig),
+ "Table using strict replica-group routing must not relocate COMPLETED
segments (tagOverrideConfig with a "
Review Comment:
Agreed. The Compatibility section now lists every config that newly fails on
its next update, split by table kind, so release notes can take it as is. One
correction on scope: for upsert and dedup tables, tenant tag overrides were
already rejected before this PR, and that check stays; the new rejection for
them is pre-configured `instancePartitionsMap.COMPLETED`. For strict-routing
tables without upsert or dedup, all three relocation paths are new.
##########
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:
Correction to the above: skipping `ALL` bypasses the table config check, but
not the controller check of the computed or fetched instance partitions, which
has never received the skip types (the same holds for the partition column
check from #19958). A follow-up reworking `ValidationType` will make that check
honor `ALL`; the description is updated accordingly.
--
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]
