You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "arunkumarucet (via GitHub)" <gi...@apache.org> on 2025/12/03 11:31:04 UTC
[PR] Move DimTable disablePreload default value from Static to cluster config based [pinot]
arunkumarucet opened a new pull request, #17313:
URL: https://github.com/apache/pinot/pull/17313
Currently by default `dimensionTableConfig.disablePreload` is set as `false` when its not configured through table config. It is now moved as a cluster config `dimTable.disablePreload.default` with same default value as `false`.
--
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] Move DimTable disablePreload default value from Static to cluster config based [pinot]
Posted by "arunkumarucet (via GitHub)" <gi...@apache.org>.
arunkumarucet commented on code in PR #17313:
URL: https://github.com/apache/pinot/pull/17313#discussion_r2587526391
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/instance/InstanceDataManagerConfig.java:
##########
@@ -90,4 +90,6 @@ public interface InstanceDataManagerConfig {
boolean isUploadSegmentToDeepStore();
boolean shouldCheckCRCOnSegmentLoad();
+
+ boolean isDimensionTableDisablePreloadEnabled();
Review Comment:
Fixed @Jackie-Jiang
--
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] Move DimTable disablePreload default value from Static to cluster config based [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #17313:
URL: https://github.com/apache/pinot/pull/17313#discussion_r2586717381
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/instance/InstanceDataManagerConfig.java:
##########
@@ -90,4 +90,6 @@ public interface InstanceDataManagerConfig {
boolean isUploadSegmentToDeepStore();
boolean shouldCheckCRCOnSegmentLoad();
+
+ boolean isDimensionTableDisablePreloadEnabled();
Review Comment:
```suggestion
boolean isDimensionTablePreloadDisabled();
```
##########
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixInstanceDataManagerConfig.java:
##########
@@ -105,6 +105,10 @@ public class HelixInstanceDataManagerConfig implements InstanceDataManagerConfig
public static final String ENABLE_ASYNC_SEGMENT_REFRESH = "enable.async.segment.refresh";
private static final boolean DEFAULT_ENABLE_ASYNC_SEGMENT_REFRESH = false;
+ // Whether to disable preloading for dimension tables. Preload Enabled by default.
+ public static final String DIMENSION_TABLE_DISABLE_PRELOAD = "dimTable.disablePreload.default";
Review Comment:
```suggestion
public static final String DISABLE_DIMENSION_TABLE_PRELOAD = "disable.dimension.table.preload";
```
--
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] Move DimTable disablePreload default value from Static to cluster config based [pinot]
Posted by "swaminathanmanish (via GitHub)" <gi...@apache.org>.
swaminathanmanish merged PR #17313:
URL: https://github.com/apache/pinot/pull/17313
--
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] Move DimTable disablePreload default value from Static to cluster config based [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #17313:
URL: https://github.com/apache/pinot/pull/17313#issuecomment-3606585748
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/17313?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 55.65%. Comparing base ([`8bfb907`](https://app.codecov.io/gh/apache/pinot/commit/8bfb907db9eeb29ea3f494f435aa2fa781378db1?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`cf64b54`](https://app.codecov.io/gh/apache/pinot/commit/cf64b54c55fdfbeabd5e9119632ee3abac4cf373?dropdown=coverage&el=desc&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 (8bfb907) and HEAD (cf64b54). Click for more details.
>
> <details><summary>HEAD has 12 uploads less than BASE</summary>
>
>| Flag | BASE (8bfb907) | HEAD (cf64b54) |
>|------|------|------|
>|unittests|3|1|
>|java-11|3|0|
>|temurin|4|1|
>|unittests2|2|0|
>|integration|1|0|
>|integration2|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #17313 +/- ##
============================================
- Coverage 63.24% 55.65% -7.60%
+ Complexity 1424 702 -722
============================================
Files 3133 2446 -687
Lines 186150 137784 -48366
Branches 28412 21894 -6518
============================================
- Hits 117737 76681 -41056
+ Misses 59351 54682 -4669
+ Partials 9062 6421 -2641
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/17313/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/17313/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/17313/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/17313/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/17313/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.65% <100.00%> (+21.76%)` | :arrow_up: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/17313/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.65% <100.00%> (-7.60%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/17313/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.65% <100.00%> (-7.60%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/17313/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.65% <100.00%> (+<0.01%)` | :arrow_up: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/17313/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/17313?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] Move DimTable disablePreload default value from Static to cluster config based [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #17313:
URL: https://github.com/apache/pinot/pull/17313#discussion_r2608760312
##########
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixInstanceDataManagerConfig.java:
##########
@@ -105,6 +105,10 @@ public class HelixInstanceDataManagerConfig implements InstanceDataManagerConfig
public static final String ENABLE_ASYNC_SEGMENT_REFRESH = "enable.async.segment.refresh";
private static final boolean DEFAULT_ENABLE_ASYNC_SEGMENT_REFRESH = false;
+ // Whether to disable preloading for dimension tables. Preload Enabled by default.
+ public static final String DISABLE_DIMENSION_TABLE_PRELOAD = "disable.dimension.table.preload";
+ private static final boolean DEFAULT_DIMENSION_TABLE_DISABLE_PRELOAD = false;
Review Comment:
For consistency
```suggestion
private static final boolean DEFAULT_DISABLE_DIMENSION_TABLE_PRELOAD = false;
```
--
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