You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "rsrkpatwari1234 (via GitHub)" <gi...@apache.org> on 2026/04/27 12:10:07 UTC
[PR] Add validation to ensure segment assignment strategy is not set as replicagroup for dimension tables [pinot]
rsrkpatwari1234 opened a new pull request, #18346:
URL: https://github.com/apache/pinot/pull/18346
Fixes #18345
Added validation to catch incorrect segment assignment strategy early and provide clear error messages.
### Why This is Backward Compatible
No existing dimension tables would have `"segmentAssignmentStrategy": "replicagroup"` set because they would have already failed with the `"Failed to find the replica-group strategy config"` error. The current behavior is fundamentally broken - any dimension table with this configuration cannot function.
The fix only affects configurations that are currently non-functional, making it completely backward compatible:
✅ Valid dimension tables (without segmentAssignmentStrategy) continue to work unchanged
❌ Invalid dimension tables (with replicagroup strategy) currently fail anyway, just with confusing errors
🔄 The fix provides clearer error messages for the same failing configurations
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add validation to ensure segment assignment strategy is either null or allservers for dimension tables [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on PR #18346:
URL: https://github.com/apache/pinot/pull/18346#issuecomment-4332691858
Thanks for the enhancement!
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add validation to ensure segment assignment strategy is either null or allservers for dimension tables [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya merged PR #18346:
URL: https://github.com/apache/pinot/pull/18346
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add validation to ensure segment assignment strategy is not set as replicagroup for dimension tables [pinot]
Posted by "rsrkpatwari1234 (via GitHub)" <gi...@apache.org>.
rsrkpatwari1234 commented on code in PR #18346:
URL: https://github.com/apache/pinot/pull/18346#discussion_r3151410861
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -371,6 +371,18 @@ private static void validateValidationConfig(TableConfig tableConfig, Schema sch
"Dimension table must be of OFFLINE table type.");
Preconditions.checkState(CollectionUtils.isNotEmpty(schema.getPrimaryKeyColumns()),
"Dimension table must have primary key[s]");
+ // Check for incompatible segment assignment strategies
+ String segmentAssignmentStrategy = validationConfig.getSegmentAssignmentStrategy();
+ if (segmentAssignmentStrategy != null
+ && CommonConstants.Segment.AssignmentStrategy
+ .REPLICA_GROUP_SEGMENT_ASSIGNMENT_STRATEGY
Review Comment:
The issue mentioned in #18345 is observed for tables with `"segmentAssignmentStrategy": "replicagroup"` and no other assignment strategy because of below check -
```
//InstanceAssignmentConfigUtils.allowInstanceAssignment():
case OFFLINE:
return tableType == TableType.OFFLINE && ((instanceAssignmentConfigMap != null
&& instanceAssignmentConfigMap.get(InstancePartitionsType.OFFLINE.toString()) != null)
|| AssignmentStrategy.REPLICA_GROUP_SEGMENT_ASSIGNMENT_STRATEGY
.equalsIgnoreCase(tableConfig.getValidationConfig().getSegmentAssignmentStrategy()));
```
Other assignment strategies apart from `replicagroup` and `allservers`
- will not result in this error
- will be successfully created in pinot.
If we add validations for these, it would be a backward incompatible change impacting all dim tables created with these configurations.
Hence we added validation for `replicagroup` assignment strategy specifically
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add validation to ensure segment assignment strategy is not set as replicagroup for dimension tables [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #18346:
URL: https://github.com/apache/pinot/pull/18346#issuecomment-4327395795
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/18346?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `0%` with `5 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 55.34%. Comparing base ([`1881884`](https://app.codecov.io/gh/apache/pinot/commit/18818842e5cfcb97ca9cc7e21f01eee5a94ef353?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`d46f476`](https://app.codecov.io/gh/apache/pinot/commit/d46f476d22fc984d18cd7f5e866e1dc1a03bc2d2?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/18346?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...he/pinot/segment/local/utils/TableConfigUtils.java](https://app.codecov.io/gh/apache/pinot/pull/18346?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Futils%2FTableConfigUtils.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC91dGlscy9UYWJsZUNvbmZpZ1V0aWxzLmphdmE=) | 0.00% | [5 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18346?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (1881884) and HEAD (d46f476). Click for more details.
>
> <details><summary>HEAD has 12 uploads less than BASE</summary>
>
>| Flag | BASE (1881884) | HEAD (d46f476) |
>|------|------|------|
>|java-21|4|1|
>|unittests|2|1|
>|temurin|4|1|
>|unittests2|1|0|
>|integration|2|0|
>|integration1|1|0|
>|custom-integration1|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #18346 +/- ##
============================================
- Coverage 63.40% 55.34% -8.07%
+ Complexity 1679 837 -842
============================================
Files 3253 2543 -710
Lines 198767 147318 -51449
Branches 30791 23736 -7055
============================================
- Hits 126034 81527 -44507
+ Misses 62659 58760 -3899
+ Partials 10074 7031 -3043
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/18346/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/18346/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/18346/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/18346/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/18346/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.34% <0.00%> (-8.07%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/18346/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.34% <0.00%> (-8.07%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/18346/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.34% <0.00%> (-8.07%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/18346/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.34% <0.00%> (-0.03%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/18346/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/18346?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add validation to ensure segment assignment strategy is not set as replicagroup for dimension tables [pinot]
Posted by "rsrkpatwari1234 (via GitHub)" <gi...@apache.org>.
rsrkpatwari1234 commented on code in PR #18346:
URL: https://github.com/apache/pinot/pull/18346#discussion_r3151471776
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -371,6 +371,18 @@ private static void validateValidationConfig(TableConfig tableConfig, Schema sch
"Dimension table must be of OFFLINE table type.");
Preconditions.checkState(CollectionUtils.isNotEmpty(schema.getPrimaryKeyColumns()),
"Dimension table must have primary key[s]");
+ // Check for incompatible segment assignment strategies
+ String segmentAssignmentStrategy = validationConfig.getSegmentAssignmentStrategy();
+ if (segmentAssignmentStrategy != null
+ && CommonConstants.Segment.AssignmentStrategy
+ .REPLICA_GROUP_SEGMENT_ASSIGNMENT_STRATEGY
Review Comment:
Agreed on the above. Making the required changes.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add validation to ensure segment assignment strategy is either null or allservers for dimension tables [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #18346:
URL: https://github.com/apache/pinot/pull/18346#issuecomment-4349877844
Opened the matching docs PR: https://github.com/pinot-contrib/pinot-docs/pull/774
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add validation to ensure segment assignment strategy is not set as replicagroup for dimension tables [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #18346:
URL: https://github.com/apache/pinot/pull/18346#discussion_r3151443861
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -371,6 +371,18 @@ private static void validateValidationConfig(TableConfig tableConfig, Schema sch
"Dimension table must be of OFFLINE table type.");
Preconditions.checkState(CollectionUtils.isNotEmpty(schema.getPrimaryKeyColumns()),
"Dimension table must have primary key[s]");
+ // Check for incompatible segment assignment strategies
+ String segmentAssignmentStrategy = validationConfig.getSegmentAssignmentStrategy();
+ if (segmentAssignmentStrategy != null
+ && CommonConstants.Segment.AssignmentStrategy
+ .REPLICA_GROUP_SEGMENT_ASSIGNMENT_STRATEGY
Review Comment:
But `balanced` isn't a valid strategy for dimension tables right? And the backward compatibility argument can also be applied to this `replicagroup` validation since it previously only resulted in warn logs and not any actual usability issues?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add validation to ensure segment assignment strategy is not set as replicagroup for dimension tables [pinot]
Posted by "rsrkpatwari1234 (via GitHub)" <gi...@apache.org>.
rsrkpatwari1234 commented on code in PR #18346:
URL: https://github.com/apache/pinot/pull/18346#discussion_r3151392604
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -371,6 +371,18 @@ private static void validateValidationConfig(TableConfig tableConfig, Schema sch
"Dimension table must be of OFFLINE table type.");
Preconditions.checkState(CollectionUtils.isNotEmpty(schema.getPrimaryKeyColumns()),
"Dimension table must have primary key[s]");
+ // Check for incompatible segment assignment strategies
+ String segmentAssignmentStrategy = validationConfig.getSegmentAssignmentStrategy();
+ if (segmentAssignmentStrategy != null
+ && CommonConstants.Segment.AssignmentStrategy
Review Comment:
Agreed. Made these changes
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add validation to ensure segment assignment strategy is not set as replicagroup for dimension tables [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #18346:
URL: https://github.com/apache/pinot/pull/18346#discussion_r3150681694
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -371,6 +371,18 @@ private static void validateValidationConfig(TableConfig tableConfig, Schema sch
"Dimension table must be of OFFLINE table type.");
Preconditions.checkState(CollectionUtils.isNotEmpty(schema.getPrimaryKeyColumns()),
"Dimension table must have primary key[s]");
+ // Check for incompatible segment assignment strategies
+ String segmentAssignmentStrategy = validationConfig.getSegmentAssignmentStrategy();
+ if (segmentAssignmentStrategy != null
+ && CommonConstants.Segment.AssignmentStrategy
+ .REPLICA_GROUP_SEGMENT_ASSIGNMENT_STRATEGY
Review Comment:
We shouldn't allow `balanced` strategy either right? So shouldn't the check be inverted to throw on any explicitly configured strategy that is not `allservers`?
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -371,6 +371,18 @@ private static void validateValidationConfig(TableConfig tableConfig, Schema sch
"Dimension table must be of OFFLINE table type.");
Preconditions.checkState(CollectionUtils.isNotEmpty(schema.getPrimaryKeyColumns()),
"Dimension table must have primary key[s]");
+ // Check for incompatible segment assignment strategies
+ String segmentAssignmentStrategy = validationConfig.getSegmentAssignmentStrategy();
+ if (segmentAssignmentStrategy != null
+ && CommonConstants.Segment.AssignmentStrategy
Review Comment:
nit: prefer import over using this qualified name
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org