Remove deprecated field segmentAssignmentStrategy in SegmentsValidationAndRetentionConfig#18398
Conversation
J-HowHuang
added
configuration
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #18398 +/- ##
=========================================
Coverage 63.53% 63.53%
Complexity 1709 1709
=========================================
Files 3250 3250
Lines 198949 198959 +10
Branches 30826 30830 +4
=========================================
+ Hits 126400 126412 +12
+ Misses 62471 62470 -1
+ Partials 10078 10077 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
J-HowHuang
force-pushed
the
remove-deprecated-table-config
branch
from
41142d2 to
15cf8eb
Compare
| "retentionTimeUnit": "DAYS", | ||
| "retentionTimeValue": "5", | ||
| "segmentPushType": "APPEND", | ||
| "segmentAssignmentStrategy": "BalanceNumSegmentAssignmentStrategy", |
There was a problem hiding this comment.
For deprecated configs, we should move those to the new configs .
E.g. https://github.com/xiangfu0/pinot/blob/2fae407497b2ad3b24fb7ef01e8820e130099904/pinot-tools/src/main/resources/examples/batch/baseballStats/baseballStats_offline_table_config.json
There was a problem hiding this comment.
I had a similar PR: #18411, would love to let you do this and I can rebase:
There was a problem hiding this comment.
This field is not consumed by anyone currently, so it should have effectively the same behavior after removing it. The SegmentAssignmentStrategyFactory#getSegmentAssignmentStrategy has been the one who figures out the strategy to use.
The new config:
"segmentAssignmentConfigMap": {
"OFFLINE": {
"segmentAssignmentStrategy": "BalanceNumSegmentAssignmentStrategy"
}
}
serves more like an override, IMO we don't need to provide them in the example table config.
yashmayya
added
the
release-notes
|
Opened docs follow-up: pinot-contrib/pinot-docs#798 This updates the dimension-table docs to use top-level |
## Summary - replace stale `validationConfig.segmentAssignmentStrategy` references with the supported top-level `segmentAssignmentConfigMap.OFFLINE.segmentAssignmentStrategy` path for dimension tables - update the dimension-table example JSON to show the supported `segmentAssignmentConfigMap` shape - fix the in-page balanced segment assignment image path while validating the edited page ## Cross-checks - verified the `apache/pinot#18398` change removed `segmentAssignmentStrategy` from `SegmentsValidationAndRetentionConfig` - verified `TableConfig` still exposes top-level `segmentAssignmentConfigMap` - verified dimension-table validation now checks only `segmentAssignmentConfigMap["OFFLINE"].segmentAssignmentStrategy` and only allows `allservers` ## Validation - `git diff --check` - lightweight relative link and asset sanity check for the edited docs files
| Map<String, SegmentAssignmentConfig> segmentAssignmentConfigMap = tableConfig.getSegmentAssignmentConfigMap(); | ||
| boolean isSetReplicaGroupAssignmentStrategy = false; | ||
| if (segmentAssignmentConfigMap != null | ||
| && segmentAssignmentConfigMap.get(instancePartitionsType.toString()) != null) { | ||
| isSetReplicaGroupAssignmentStrategy = | ||
| AssignmentStrategy.REPLICA_GROUP_SEGMENT_ASSIGNMENT_STRATEGY.equalsIgnoreCase( | ||
| segmentAssignmentConfigMap.get(instancePartitionsType.toString()).getAssignmentStrategy()); | ||
| } |
There was a problem hiding this comment.
Why do we need this logic?
There was a problem hiding this comment.
This is to see if the table has explicitly configured REPLICA_GROUP_SEGMENT_ASSIGNMENT_STRATEGY in segment assignment strategy.
I don't know whether it's common to have this strategy explicitly set in segment assignment strategy, but this is the same logic as before, just to check from the segment assignment strategy map instead of segment assignment strategy in segment validation config (the deprecated field).
There was a problem hiding this comment.
Line 78 already covers this. When deprecating a field, in most cases we don't need to add new logic
There was a problem hiding this comment.
We always allow instance assignment when instanceAssignmentConfig is configured. It is not tight to replica group assignment strategy
There was a problem hiding this comment.
It was this part of the original code
|| AssignmentStrategy.REPLICA_GROUP_SEGMENT_ASSIGNMENT_STRATEGY
.equalsIgnoreCase(tableConfig.getValidationConfig().getSegmentAssignmentStrategy()));
And we're deprecating this so I changed this to its counterpart, which is the segmentAssignmentConfigMap.
Also correctness-wise, in SegmentAssignmentStrategyFactory#getSegmentAssignmentStrategy, if a table doesn't have instanceAssignmentConfig but has the strategy explicitly set in segmentAssignmentConfigMap, it would use replica group strategy, so this check is relevant in this sense.
There was a problem hiding this comment.
I just realized this is segment assignment config, not instance assignment config.. But this is wrong coupling. Segment assignment config shouldn't interfere with instance assignment. The old AssignmentStrategy.REPLICA_GROUP_SEGMENT_ASSIGNMENT_STRATEGY has implicit meaning on both instance assignment and segment assignment, but segment assignment shouldn't be coupled with instance assignment.
Segment assignment config is very rarely used (I even forgot its existence). We should also modify all tests to explicitly put instance assignment config instead of segment assignment config.
5 participants
Description
This field has been marked deprecated since #11869, and has not been used in pinot code effectively.
Backward Incompatibility
For any downstream usage of this deprecated field
segmentAssignmentStrategyinsegmentConfig, please read fromsegmentAssignmentConfigMapfrom first level field from table config instead. Related issue: #13659