You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "karthi07 (via GitHub)" <gi...@apache.org> on 2024/03/20 16:52:47 UTC
[PR] 12508: Feature add segment rows flush config [pinot]
karthi07 opened a new pull request, #12681:
URL: https://github.com/apache/pinot/pull/12681
Adding new config "realtime.segment.flush.threshold.segment.rows" for real-time table to configure the segment threshold independent of max partitions
**Assumptions:**
- If flush size > 0, create a new DefaultFlushThresholdUpdater with given flush size.
- If flush size <= 0, create new SegmentRowsBasedFlushThresholdUpdater if flushThresholdSegmentRows > 0.
- If flush size <= 0 AND segment.row <=0, create new SegmentSizeBasedFlushThresholdUpdater if not already created.
**Default values**
- when value is not provided -> value is considered as 0
- when invalid config is provided -> 5000000 will be considered.
**Issue[12508]:**
https://github.com/apache/pinot/issues/12508
When realtime.segment.flush.threshold.rows is used, the flush threshold for the new CONSUMING segment is determined by both this value and the max partitions consumed by any server. This is not very straight forward, and rebalancing a table could cause new consuming segment size to change.
In order to tackle this problem, we may add a new config (proposing realtime.segment.flush.threshold.segment.rows) which works independent of partitions consumed.
--
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] 12508: Feature add segment rows flush config [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #12681:
URL: https://github.com/apache/pinot/pull/12681
--
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] 12508: Feature add segment rows flush config [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #12681:
URL: https://github.com/apache/pinot/pull/12681#issuecomment-2014907773
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/12681?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `65.38462%` with `9 lines` in your changes are missing coverage. Please review.
> Project coverage is 61.40%. Comparing base [(`59551e4`)](https://app.codecov.io/gh/apache/pinot/commit/59551e45224f1535c4863fd577622b37366ccc97?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`3494dbf`)](https://app.codecov.io/gh/apache/pinot/pull/12681?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 156 commits behind head on master.
| [Files](https://app.codecov.io/gh/apache/pinot/pull/12681?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...java/org/apache/pinot/spi/stream/StreamConfig.java](https://app.codecov.io/gh/apache/pinot/pull/12681?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvc3RyZWFtL1N0cmVhbUNvbmZpZy5qYXZh) | 43.75% | [8 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/pinot/pull/12681?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #12681 +/- ##
============================================
- Coverage 61.75% 61.40% -0.35%
+ Complexity 207 198 -9
============================================
Files 2436 2457 +21
Lines 133233 134424 +1191
Branches 20636 20806 +170
============================================
+ Hits 82274 82543 +269
- Misses 44911 45709 +798
- Partials 6048 6172 +124
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/12681/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/12681/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `<0.01% <0.00%> (-0.01%)` | :arrow_down: |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/12681/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `<0.01% <0.00%> (-0.01%)` | :arrow_down: |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/12681/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `<0.01% <0.00%> (-0.01%)` | :arrow_down: |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/12681/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (ø)` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/12681/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `27.90% <38.46%> (-33.81%)` | :arrow_down: |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/12681/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `61.40% <65.38%> (-0.23%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/12681/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `61.38% <65.38%> (-0.37%)` | :arrow_down: |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/12681/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `61.37% <65.38%> (+33.64%)` | :arrow_up: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/12681/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `61.40% <65.38%> (-0.35%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/12681/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `61.40% <65.38%> (-0.35%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/12681/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.00% <43.75%> (-0.89%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/12681/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `27.91% <38.46%> (+0.18%)` | :arrow_up: |
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/12681?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).
--
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] 12508: Feature add segment rows flush config [pinot]
Posted by "karthi07 (via GitHub)" <gi...@apache.org>.
karthi07 commented on PR #12681:
URL: https://github.com/apache/pinot/pull/12681#issuecomment-2022431101
@Jackie-Jiang I updated lines related to thresholdUpdateManager and Its test file
I lost a little while merging with #12684, Please review the files and let me the changes required.
--
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] 12508: Feature add segment rows flush config [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #12681:
URL: https://github.com/apache/pinot/pull/12681#issuecomment-2011387627
#12684 cleans up the current threshold handling
--
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] 12508: Feature add segment rows flush config [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #12681:
URL: https://github.com/apache/pinot/pull/12681#issuecomment-2019099829
@karthi07 Just merged #12684. Can you please rebase this PR on top of that?
--
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] 12508: Feature add segment rows flush config [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #12681:
URL: https://github.com/apache/pinot/pull/12681#discussion_r1533284146
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/segment/FlushThresholdUpdateManager.java:
##########
@@ -33,16 +33,20 @@ public class FlushThresholdUpdateManager {
* Check table config for flush size.
*
* If flush size > 0, create a new DefaultFlushThresholdUpdater with given flush size.
- * If flush size <= 0, create new SegmentSizeBasedFlushThresholdUpdater if not already created. Create only 1 per
- * table because we want to maintain tuning information for the table in the updater.
+ * If flush size <= 0, create new SegmentRowsBasedFlushThresholdUpdater if flushThresholdSegmentRows > 0.
+ * If flush size <= 0 AND segment.row <=0, create new SegmentSizeBasedFlushThresholdUpdater if not already created.
+ * Create only 1 per table because we want to maintain tuning information for the table in the updater.
*/
public FlushThresholdUpdater getFlushThresholdUpdater(StreamConfig streamConfig) {
String realtimeTableName = streamConfig.getTableNameWithType();
int flushThresholdRows = streamConfig.getFlushThresholdRows();
+ int flushThresholdSegmentRows = streamConfig.getFlushThresholdSegmentRows();
if (flushThresholdRows > 0) {
_flushThresholdUpdaterMap.remove(realtimeTableName);
return new DefaultFlushThresholdUpdater(flushThresholdRows);
+ } else if (flushThresholdSegmentRows > 0) {
+ return new SegmentRowsBasedFlushThresholdUpdater(flushThresholdSegmentRows);
Review Comment:
Suggest renaming it to `FixedFlushThresholdUpdater`. You also need to call `_flushThresholdUpdaterMap.remove(realtimeTableName);`
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/segment/FlushThresholdUpdateManager.java:
##########
@@ -33,16 +33,20 @@ public class FlushThresholdUpdateManager {
* Check table config for flush size.
*
* If flush size > 0, create a new DefaultFlushThresholdUpdater with given flush size.
- * If flush size <= 0, create new SegmentSizeBasedFlushThresholdUpdater if not already created. Create only 1 per
- * table because we want to maintain tuning information for the table in the updater.
+ * If flush size <= 0, create new SegmentRowsBasedFlushThresholdUpdater if flushThresholdSegmentRows > 0.
Review Comment:
We need to revise the javadoc here. Currently it is not clear.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/stream/StreamConfigProperties.java:
##########
@@ -81,6 +81,7 @@ private StreamConfigProperties() {
*/
public static final String DEPRECATED_SEGMENT_FLUSH_THRESHOLD_ROWS = "realtime.segment.flush.threshold.size";
public static final String SEGMENT_FLUSH_THRESHOLD_ROWS = "realtime.segment.flush.threshold.rows";
+ public static final String SEGMENT_FLUSH_THRESHOLD_SEGMENT_ROWS = "realtime.segment.flush.threshold.segment.rows";
Review Comment:
Please add some javadoc explaining the difference between this and `SEGMENT_FLUSH_THRESHOLD_ROWS`
--
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] 12508: Feature add segment rows flush config [pinot]
Posted by "karthi07 (via GitHub)" <gi...@apache.org>.
karthi07 commented on PR #12681:
URL: https://github.com/apache/pinot/pull/12681#issuecomment-2014830665
@Jackie-Jiang Your suggestions were helpful, I updated the docs and renamed the FixedThreshold. let me know if the docs need additional changes.
Should I merge #12684 into my branch and update the default threshold calc, or should I wait till it gets merged into master?
--
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