You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "manakbisht (via GitHub)" <gi...@apache.org> on 2026/10/26 14:13:38 UTC
[PR] Add per-table hybrid retention strategy with cluster-level fallback [pinot]
manakbisht opened a new pull request, #20057:
URL: https://github.com/apache/pinot/pull/20057
## Problem
- The retention strategy for hybrid tables can only be configured at the cluster level. This is inflexible, as it can’t be enabled selectively per table.
## Changes
- Tables can now set their own hybrid retention strategy (`segmentsConfig.enableHybridTableRetentionStrategy`). If unset (default), the cluster-level config applies, which currently defaults to false.
- Deprecate the cluster-level config (`controller.enable.hybrid.table.retention.strategy`) for removal in a future release.
## Tests
- Modified `TableConfigSerDeUtilsTest#testSerDe` for serialization-deserialization of new field.
- Added new test `RetentionManagerTest#testTableLevelHybridRetentionStrategyOverridesClusterConfig` to cover configuration overrides.
--
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 per-table hybrid retention strategy with cluster-level fallback [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #20057:
URL: https://github.com/apache/pinot/pull/20057#discussion_r4381344184
##########
pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java:
##########
@@ -394,6 +394,9 @@ public static long getRandomInitialDelayInSeconds() {
// the controller as ready. When enabled, controller.resource.utilization.checker.initial.delay is set to 0.
public static final String RESOURCE_UTILIZATION_CHECKER_COLLECT_USAGE_AT_STARTUP =
"controller.resource.utilization.checker.collect.usage.at.startup";
+ /// @deprecated Use `segmentsConfig.enableHybridTableRetentionStrategy` in the table config instead (see
+ /// [org.apache.pinot.spi.config.table.SegmentsValidationAndRetentionConfig#getEnableHybridTableRetentionStrategy()]).
+ @Deprecated(since = "1.6.0", forRemoval = true)
Review Comment:
Don't deprecate this config. We want to allow cluster config to set the default if it is not configured at table level
--
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 per-table hybrid retention strategy with cluster-level fallback [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #20057:
URL: https://github.com/apache/pinot/pull/20057#discussion_r4378828496
##########
pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java:
##########
@@ -394,6 +394,9 @@ public static long getRandomInitialDelayInSeconds() {
// the controller as ready. When enabled, controller.resource.utilization.checker.initial.delay is set to 0.
public static final String RESOURCE_UTILIZATION_CHECKER_COLLECT_USAGE_AT_STARTUP =
"controller.resource.utilization.checker.collect.usage.at.startup";
+ /// @deprecated Use `segmentsConfig.enableHybridTableRetentionStrategy` in the table config instead (see
+ /// [org.apache.pinot.spi.config.table.SegmentsValidationAndRetentionConfig#getEnableHybridTableRetentionStrategy()]).
Review Comment:
Small nit: `since` marks the version where deprecation was introduced, which is the current dev line `1.6.0` (that's the convention used elsewhere in the repo). `1.7.0` reads more like the intended removal release. Suggest `since = "1.6.0"`.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/retention/RetentionManager.java:
##########
@@ -360,7 +361,10 @@ private void manageRetentionForTable(TableConfig tableConfig) {
String rawTableName = TableNameBuilder.extractRawTableName(tableNameWithType);
TableConfig offlineTableConfig = _pinotHelixResourceManager.getOfflineTableConfig(rawTableName);
// hybrid table check should be performed before the realtime table check.
- if (_isHybridTableRetentionStrategyEnabled && offlineTableConfig != null) {
+ boolean hybridTableRetentionStrategyEnabled =
+ Objects.requireNonNullElse(validationConfig.getEnableHybridTableRetentionStrategy(),
+ _isHybridTableRetentionStrategyEnabled);
+ if (hybridTableRetentionStrategyEnabled && offlineTableConfig != null) {
Review Comment:
This override is only read from the REALTIME table config (this branch is realtime-only; the offline config here is used solely for the time boundary). An operator who sets `enableHybridTableRetentionStrategy` on the OFFLINE config of a hybrid table gets it silently ignored. Worth documenting the realtime-only requirement, or logging when the offline config carries a conflicting non-null value.
--
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 per-table hybrid retention strategy with cluster-level fallback [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #20057:
URL: https://github.com/apache/pinot/pull/20057#discussion_r4381360781
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/SegmentsValidationAndRetentionConfig.java:
##########
@@ -55,6 +56,7 @@ public class SegmentsValidationAndRetentionConfig extends BaseJsonConfig {
private String _untrackedSegmentsDeletionBatchSize;
private String _untrackedSegmentsRetentionTimeUnit;
private String _untrackedSegmentsRetentionTimeValue;
+ private Boolean _enableHybridTableRetentionStrategy;
Review Comment:
Consider store it as `Enablement`, and move it next to the retention setting configs
--
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 per-table hybrid retention strategy with cluster-level fallback [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #20057:
URL: https://github.com/apache/pinot/pull/20057#issuecomment-6428749004
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/20057?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 59.09%. Comparing base ([`c5b8107`](https://app.codecov.io/gh/apache/pinot/commit/c5b8107fa6a0e0bcf7f1b401cc2aa827a7e32e74?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`7ce3530`](https://app.codecov.io/gh/apache/pinot/commit/7ce3530b534fc88b792dbdc34e457755d96d61d5?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 14 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #20057 +/- ##
=============================================
+ Coverage 41.85% 59.09% +17.24%
+ Complexity 1518 7 -1511
=============================================
Files 3558 2746 -812
Lines 236346 173355 -62991
Branches 37954 28615 -9339
=============================================
+ Hits 98924 102448 +3524
+ Misses 128630 62254 -66376
+ Partials 8792 8653 -139
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/20057/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [integration](https://app.codecov.io/gh/apache/pinot/pull/20057/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (ø)` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/20057/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (ø)` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/20057/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (?)` | |
| [java-25](https://app.codecov.io/gh/apache/pinot/pull/20057/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `59.09% <100.00%> (+17.24%)` | :arrow_up: |
| [lane-a](https://app.codecov.io/gh/apache/pinot/pull/20057/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (ø)` | |
| [lane-b](https://app.codecov.io/gh/apache/pinot/pull/20057/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (ø)` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/20057/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `59.09% <100.00%> (+17.24%)` | :arrow_up: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/20057/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `59.09% <100.00%> (+17.24%)` | :arrow_up: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/20057/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `59.09% <100.00%> (?)` | |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/20057/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 Harness](https://app.codecov.io/gh/apache/pinot/pull/20057?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