xiangfu0 opened a new pull request, #19987:
URL: https://github.com/apache/pinot/pull/19987
**Cherry-pick request:** `release-1.6.0-rc`. apache/pinot#18398 is on the
rc, so 1.6.0 would ship with the regression below; a trial `git cherry-pick` of
this commit onto the rc applies with no conflicts.
## Summary
apache/pinot#18398 removed the deprecated
`segmentsConfig.segmentAssignmentStrategy` field from
`SegmentsValidationAndRetentionConfig` and, with it, the legacy branch of
`InstanceAssignmentConfigUtils.allowInstanceAssignment(tableConfig, OFFLINE)`.
On release-1.5.1 that branch returned true when the key equals `replicagroup`
ignoring case
(`CommonConstants.Segment.AssignmentStrategy.REPLICA_GROUP_SEGMENT_ASSIGNMENT_STRATEGY`,
that value since apache/pinot#9309 in 0.12.0). On master only
`segmentAssignmentConfigMap.OFFLINE.assignmentStrategy == replicagroup` counts.
This PR restores the field as a deprecated field and the legacy branch of
the gate, so the key can go through a proper deprecation lifecycle (announce,
warn, remove) instead of being dropped silently in 1.6.0.
## Hazard
For an OFFLINE table whose replica-group instance assignment is configured
only through the legacy key (no `instanceAssignmentConfigMap`, no
`segmentAssignmentConfigMap`), the gate flips from true to false after
upgrading to 1.6.0. `TableRebalancer.getInstancePartitions()` then takes the
"instance assignment is not allowed" branch whenever `reassignInstances=true`:
it computes default (non-replica-group) instance partitions, **removes the
table's persisted InstancePartitions from ZK**, and reassigns every segment
across the tenant. `reassignInstances` defaults to `true` on the REST rebalance
endpoint (`PinotTableRestletResource`), on `pinot-admin RebalanceTable` and in
the UI. The same gate also decides whether
`PinotHelixResourceManager.assignInstances()` persists InstancePartitions on
table creation and update.
## Changes
- `pinot-spi` `SegmentsValidationAndRetentionConfig`: restore
`_segmentAssignmentStrategy` with its getter and setter, as on release-1.5.1.
The getter is `@Deprecated(since = "1.1.0", forRemoval = true)` (release-1.1.0
is the first tag with `@Deprecated` on the field) and its Javadoc points at
`segmentAssignmentConfigMap`.
- `pinot-common` `InstanceAssignmentConfigUtils.allowInstanceAssignment`:
restore `||
REPLICA_GROUP_SEGMENT_ASSIGNMENT_STRATEGY.equalsIgnoreCase(validationConfig.getSegmentAssignmentStrategy())`
next to the existing `segmentAssignmentConfigMap` check.
- `pinot-segment-local` `TableConfigUtils.validateValidationConfig`: restore
the legacy-key branch of the dimension-table strategy check from
apache/pinot#18346, which #18398 rewrote to read only
`segmentAssignmentConfigMap`. With the key readable again, both ways of setting
the strategy are validated.
- Deliberately not restored (no behaviour):
`TableConfigBuilder.setSegmentAssignmentStrategy` (writer only; tests set the
key on the validation config directly), the unused `PerfBenchmarkDriver`
constant, and the `"segmentAssignmentStrategy":
"BalanceNumSegmentAssignmentStrategy"` lines in sample and test JSON configs.
## Tests
-
`InstanceAssignmentConfigUtilsTest#testAllowInstanceAssignmentWithLegacySegmentAssignmentStrategy`:
legacy key alone yields `allowInstanceAssignment(OFFLINE) == true`,
case-insensitively; other strategies and REALTIME tables stay false.
- `TableConfigSerDeUtilsTest#testSerDe`: the key survives JSON and ZNRecord
round trips and parses from JSON written by an older release.
- `TableConfigUtilsTest#validateDimensionTableSegmentAssignmentStrategy`:
legacy-key cases for the dimension-table check.
-
`TableRebalancerClusterStatelessTest#testRebalanceWithLegacySegmentAssignmentStrategy`:
rebalancing such a table with `reassignInstances=true` keeps the persisted
replica-group InstancePartitions and returns NO_OP instead of removing them.
The test fails on master without the fix.
## Upgrade note
Without this change, upgrading to 1.6.0 and rebalancing an OFFLINE table
whose replica-group assignment is configured only through the deprecated
`segmentsConfig.segmentAssignmentStrategy` key with the default
`reassignInstances=true` (REST `POST /tables/{tableName}/rebalance`,
`pinot-admin RebalanceTable`, and the UI) removes the table's persisted
instance partitions and moves every segment across the tenant. The key stays
deprecated; migrate such tables to `instanceAssignmentConfigMap` /
`segmentAssignmentConfigMap` before it is removed in a future release.
The touched gate file is identical on `release-1.6.0-rc`; the config class
differs there only in unrelated retention code.
--
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]
