You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "musclemuller (via GitHub)" <gi...@apache.org> on 2025/08/04 21:49:57 UTC
[PR] check pauseless comsumption when multi-topic ingest [pinot]
musclemuller opened a new pull request, #16507:
URL: https://github.com/apache/pinot/pull/16507
The pr is for checking pauseless consumption would be disabled with multi-topic ingestion:
https://github.com/apache/pinot/issues/16451
--
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] check pauseless comsumption when multi-topic ingest [pinot]
Posted by "musclemuller (via GitHub)" <gi...@apache.org>.
musclemuller commented on code in PR #16507:
URL: https://github.com/apache/pinot/pull/16507#discussion_r2264125568
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/utils/TableConfigUtilsTest.java:
##########
@@ -815,6 +815,57 @@ public void ingestionStreamConfigsTest() {
// Legacy behavior: allow size based threshold to be explicitly set to 0
streamConfigs.put(StreamConfigProperties.SEGMENT_FLUSH_THRESHOLD_ROWS, "0");
TableConfigUtils.validateStreamConfig(new StreamConfig("test", streamConfigs));
+
Review Comment:
I think multi topic ingestion config only supported in ingestionconfig, and it's not supported in the indexingConfig. so i validated only in this test (ingestionConfig).
--
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] check pauseless comsumption when multi-topic ingest [pinot]
Posted by "musclemuller (via GitHub)" <gi...@apache.org>.
musclemuller commented on code in PR #16507:
URL: https://github.com/apache/pinot/pull/16507#discussion_r2258426483
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/utils/TableConfigUtilsTest.java:
##########
@@ -815,6 +815,57 @@ public void ingestionStreamConfigsTest() {
// Legacy behavior: allow size based threshold to be explicitly set to 0
streamConfigs.put(StreamConfigProperties.SEGMENT_FLUSH_THRESHOLD_ROWS, "0");
TableConfigUtils.validateStreamConfig(new StreamConfig("test", streamConfigs));
+
Review Comment:
I see, thanks for remind, would remove this
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -622,6 +622,13 @@ private static void validateStreamConfigMaps(TableConfig tableConfig) {
streamConfigs.add(streamConfig);
}
if (numStreamConfigs > 1) {
+ IngestionConfig ingestionConfig = tableConfig.getIngestionConfig();
Review Comment:
addressed
--
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] check pauseless comsumption when multi-topic ingest [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #16507:
URL: https://github.com/apache/pinot/pull/16507#issuecomment-3152755023
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/16507?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 `15 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 56.31%. Comparing base ([`e59aa47`](https://app.codecov.io/gh/apache/pinot/commit/e59aa47c3525cdde3e0d8db3bb31fa874ab30cfb?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`5a0c039`](https://app.codecov.io/gh/apache/pinot/commit/5a0c039539182180dcd7fb67879637dc618af35a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 13 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/16507?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/16507?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% | [14 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/pinot/pull/16507?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 (e59aa47) and HEAD (5a0c039). Click for more details.
>
> <details><summary>HEAD has 32 uploads less than BASE</summary>
>
>| Flag | BASE (e59aa47) | HEAD (5a0c039) |
>|------|------|------|
>|unittests2|2|0|
>|unittests|3|1|
>|java-21|4|1|
>|temurin|9|1|
>|java-11|5|0|
>|integration|6|0|
>|integration1|2|0|
>|custom-integration1|2|0|
>|integration2|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #16507 +/- ##
============================================
- Coverage 63.22% 56.31% -6.91%
+ Complexity 1359 699 -660
============================================
Files 3009 2354 -655
Lines 174149 129566 -44583
Branches 26649 20604 -6045
============================================
- Hits 110105 72966 -37139
+ Misses 55647 50572 -5075
+ Partials 8397 6028 -2369
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/16507/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/16507/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/16507/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/16507/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/16507/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/16507/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/16507/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.31% <0.00%> (+23.05%)` | :arrow_up: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/16507/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.31% <0.00%> (-6.91%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/16507/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.31% <0.00%> (-6.91%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/16507/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.31% <0.00%> (-0.06%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/16507/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/16507?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] check pauseless comsumption when multi-topic ingest [pinot]
Posted by "musclemuller (via GitHub)" <gi...@apache.org>.
musclemuller commented on code in PR #16507:
URL: https://github.com/apache/pinot/pull/16507#discussion_r2258360987
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -223,7 +223,7 @@ public static void validateInstancePoolsNReplicaGroups(TableConfig tableConfig)
private static Set<ValidationType> parseTypesToSkipString(@Nullable String typesToSkip) {
return typesToSkip == null ? Collections.emptySet()
- : Arrays.stream(typesToSkip.split(",")).map(s -> ValidationType.valueOf(s.toUpperCase()))
+ : Arrays.stream(typesToSkip.split(",")).map(s -> ValidationType.valueOf(s.toUpperCase()))
Review Comment:
thanks for addressing!
--
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] check pauseless comsumption when multi-topic ingest [pinot]
Posted by "deemoliu (via GitHub)" <gi...@apache.org>.
deemoliu commented on code in PR #16507:
URL: https://github.com/apache/pinot/pull/16507#discussion_r2258385870
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/utils/TableConfigUtilsTest.java:
##########
@@ -815,6 +815,57 @@ public void ingestionStreamConfigsTest() {
// Legacy behavior: allow size based threshold to be explicitly set to 0
streamConfigs.put(StreamConfigProperties.SEGMENT_FLUSH_THRESHOLD_ROWS, "0");
TableConfigUtils.validateStreamConfig(new StreamConfig("test", streamConfigs));
+
Review Comment:
streamconfigMap validation should not only be test in ingestionStreamConfigsTest().
this is because streamconfigMap can be either in ingestionconfig or indexingConfig.
see https://github.com/apache/pinot/blob/78dabced284493933d526e2fce61f239443bf5f1/pinot-spi/src/main/java/org/apache/pinot/spi/utils/IngestionConfigUtils.java#L71C5-L71C87
--
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] check pauseless comsumption when multi-topic ingest [pinot]
Posted by "deemoliu (via GitHub)" <gi...@apache.org>.
deemoliu commented on code in PR #16507:
URL: https://github.com/apache/pinot/pull/16507#discussion_r2252789001
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -222,9 +222,9 @@ public static void validateInstancePoolsNReplicaGroups(TableConfig tableConfig)
}
private static Set<ValidationType> parseTypesToSkipString(@Nullable String typesToSkip) {
- return typesToSkip == null ? Collections.emptySet()
- : Arrays.stream(typesToSkip.split(",")).map(s -> ValidationType.valueOf(s.toUpperCase()))
- .collect(Collectors.toSet());
+ return typesToSkip == null ? Collections.emptySet() : Arrays.stream(typesToSkip.split(","))
+ .map(s -> ValidationType.valueOf(s.toUpperCase()))
Review Comment:
why format has been changed for these lines?
--
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] check pauseless comsumption when multi-topic ingest [pinot]
Posted by "deemoliu (via GitHub)" <gi...@apache.org>.
deemoliu commented on code in PR #16507:
URL: https://github.com/apache/pinot/pull/16507#discussion_r2258350491
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -223,7 +223,7 @@ public static void validateInstancePoolsNReplicaGroups(TableConfig tableConfig)
private static Set<ValidationType> parseTypesToSkipString(@Nullable String typesToSkip) {
return typesToSkip == null ? Collections.emptySet()
- : Arrays.stream(typesToSkip.split(",")).map(s -> ValidationType.valueOf(s.toUpperCase()))
+ : Arrays.stream(typesToSkip.split(",")).map(s -> ValidationType.valueOf(s.toUpperCase()))
Review Comment:
let's use pinot checkstyle and revert unrelated format 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] check pauseless comsumption when multi-topic ingest [pinot]
Posted by "deemoliu (via GitHub)" <gi...@apache.org>.
deemoliu commented on PR #16507:
URL: https://github.com/apache/pinot/pull/16507#issuecomment-3176896340
confirmed that multi-topic ingestion config only added to ingestionConfig, and indexingConfig will be deprecated soon so we don't need a test to cover it.
@Jackie-Jiang can you please verify if this change break any use cases from Startree?
--
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] check pauseless comsumption when multi-topic ingest [pinot]
Posted by "deemoliu (via GitHub)" <gi...@apache.org>.
deemoliu merged PR #16507:
URL: https://github.com/apache/pinot/pull/16507
--
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] check pauseless comsumption when multi-topic ingest [pinot]
Posted by "deemoliu (via GitHub)" <gi...@apache.org>.
deemoliu commented on code in PR #16507:
URL: https://github.com/apache/pinot/pull/16507#discussion_r2258385870
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/utils/TableConfigUtilsTest.java:
##########
@@ -815,6 +815,57 @@ public void ingestionStreamConfigsTest() {
// Legacy behavior: allow size based threshold to be explicitly set to 0
streamConfigs.put(StreamConfigProperties.SEGMENT_FLUSH_THRESHOLD_ROWS, "0");
TableConfigUtils.validateStreamConfig(new StreamConfig("test", streamConfigs));
+
Review Comment:
streamconfigMap validation should not be test in ingestionStreamConfigsTest().
this is because streamconfigMap can be either in ingestionconfig or indexingConfig.
see https://github.com/apache/pinot/blob/78dabced284493933d526e2fce61f239443bf5f1/pinot-spi/src/main/java/org/apache/pinot/spi/utils/IngestionConfigUtils.java#L71C5-L71C87
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -622,6 +622,13 @@ private static void validateStreamConfigMaps(TableConfig tableConfig) {
streamConfigs.add(streamConfig);
}
if (numStreamConfigs > 1) {
+ IngestionConfig ingestionConfig = tableConfig.getIngestionConfig();
Review Comment:
update to
if (ingestionConfig != null && ingestionConfig.getStreamIngestionConfig() != null) {
Preconditions.checkState(
!tableConfig.getIngestionConfig().getStreamIngestionConfig().isPauselessConsumptionEnabled(),
"Multiple stream configs are not supported with pauseless consumption enabled");
}
--
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] check pauseless comsumption when multi-topic ingest [pinot]
Posted by "musclemuller (via GitHub)" <gi...@apache.org>.
musclemuller commented on code in PR #16507:
URL: https://github.com/apache/pinot/pull/16507#discussion_r2255843081
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -222,9 +222,9 @@ public static void validateInstancePoolsNReplicaGroups(TableConfig tableConfig)
}
private static Set<ValidationType> parseTypesToSkipString(@Nullable String typesToSkip) {
- return typesToSkip == null ? Collections.emptySet()
- : Arrays.stream(typesToSkip.split(",")).map(s -> ValidationType.valueOf(s.toUpperCase()))
- .collect(Collectors.toSet());
+ return typesToSkip == null ? Collections.emptySet() : Arrays.stream(typesToSkip.split(","))
+ .map(s -> ValidationType.valueOf(s.toUpperCase()))
Review Comment:
it's changed by auto reformat, has already rollback these irrelevant cahnges
--
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