xiangfu0 commented on code in PR #20006:
URL: https://github.com/apache/pinot/pull/20006#discussion_r4355134934
##########
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:
Minor edge case: for the deprecated `replicaGroupStrategyConfig` with no
partition column, `getInstanceAssignmentConfig` hardcodes
`numInstancesPerPartition = 0` (it puts the count into
`numInstancesPerReplicaGroup`), so this `== 1` check always fails for that
path. For `numInstancesPerReplicaGroup > 1` rejecting is correct, but `new
ReplicaGroupStrategyConfig(null, 1)` yields a valid
one-instance-per-replica-group layout yet gets rejected with a message saying
to set it to 1 when it effectively is. It's a narrow (and untested) branch, but
worth either special-casing `partitionColumn == null &&
numInstancesPerReplicaGroup == 1` or at least noting it.
##########
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:
This runs on every table update too
(`PinotTableRestletResource.updateTableConfig` -> `validate`), and it sits
outside the `if (!skipTypes.contains(UPSERT))` block, so an upsert/dedup table
that previously relied on `validationTypesToSkip=UPSERT` to edit an unrelated
field can now only get past it with `ALL`, which disables all validation. It
also doesn't take `existingTableConfig`, so there's no grandfathering like
`validateIngestionConfig` does. Since the PR is intentionally backward-incompat
this may be fine, but could you confirm it's deliberate that the only escape
hatch for an already-deployed non-conforming table is `ALL`? A dedicated skip
type or grandfathering on `existingTableConfig` would soften the blast radius.
--
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]
