You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "rseetham (via GitHub)" <gi...@apache.org> on 2025/08/23 05:55:14 UTC
[PR] Enhance /tableConfigs/validate endpoint with cluster-aware validations [pinot]
rseetham opened a new pull request, #16675:
URL: https://github.com/apache/pinot/pull/16675
## Description
This enhances the `/tableConfigs/validate` endpoint to include cluster-aware validations that were previously only performed during table creation.
Currently, some validation errors (like "Failed to find instances with tag: X") only show up in logs during table creation, which makes debugging harder. This change moves those validations to the validation endpoint so you get immediate feedback.
## Changes
- Added new validation types: `TENANT`, `MINION_INSTANCES`, `ACTIVE_TASKS`
- Enhanced `/tableConfigs/validate` to check cluster state by default
- Added tenant/minion instance validation
- Support skipping validations via `validationTypesToSkip` parameter
- Skip cluster validations in `addConfig`/`updateConfig` to avoid duplication
- Added unit tests for the new functionality
This enables fail-fast validation and better error visibility during development.
--
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] Enhance /tableConfigs/validate endpoint with cluster-aware validations [pinot]
Posted by "tarun11Mavani (via GitHub)" <gi...@apache.org>.
tarun11Mavani commented on code in PR #16675:
URL: https://github.com/apache/pinot/pull/16675#discussion_r2295461228
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/TableConfigsRestletResource.java:
##########
@@ -210,7 +216,7 @@ public ConfigSuccessResponse addConfig(
}
TableConfigs tableConfigs = tableConfigsAndUnrecognizedProps.getLeft();
String databaseName = DatabaseUtils.extractDatabaseFromHttpHeaders(httpHeaders);
- validateConfig(tableConfigs, databaseName, typesToSkip);
+ validateConfig(tableConfigs, databaseName, SKIP_CLUSTER_VALIDATIONS);
Review Comment:
If we hardcode this, users will not be able to pass typesToSkip. We should avoid this change or have a logic to add SKIP_CLUSTER_VALIDATIONS into typesToSkip before making the call.
IMO we should keep it as is and let it do validation twice.
--
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] Enhance /tableConfigs/validate endpoint with cluster-aware validations [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #16675:
URL: https://github.com/apache/pinot/pull/16675#issuecomment-3216441023
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/16675?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 `1 line` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 56.40%. Comparing base ([`1a476de`](https://app.codecov.io/gh/apache/pinot/commit/1a476dea72ebf1c15e14ded28ae60c385f71b2c3?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`6752e37`](https://app.codecov.io/gh/apache/pinot/commit/6752e37e3267f7307fbb2601228a350baa26415c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 676 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/16675?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/16675?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% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/16675?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 (1a476de) and HEAD (6752e37). Click for more details.
>
> <details><summary>HEAD has 68 uploads less than BASE</summary>
>
>| Flag | BASE (1a476de) | HEAD (6752e37) |
>|------|------|------|
>|java-21|10|1|
>|unittests|6|1|
>|unittests1|3|1|
>|skip-bytebuffers-true|4|0|
>|temurin|15|1|
>|unittests2|3|0|
>|skip-bytebuffers-false|8|0|
>|java-11|5|0|
>|integration|9|0|
>|integration2|3|0|
>|integration1|3|0|
>|custom-integration1|3|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #16675 +/- ##
============================================
- Coverage 62.90% 56.40% -6.50%
+ Complexity 1386 699 -687
============================================
Files 2867 2379 -488
Lines 163354 131777 -31577
Branches 24952 21000 -3952
============================================
- Hits 102755 74333 -28422
+ Misses 52847 51274 -1573
+ Partials 7752 6170 -1582
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/16675/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/16675/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/16675/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/16675/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/16675/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/16675/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/16675/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.40% <0.00%> (-6.42%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/16675/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/16675/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/16675/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.40% <0.00%> (-6.50%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/16675/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.40% <0.00%> (-6.50%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/16675/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.40% <0.00%> (+0.58%)` | :arrow_up: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/16675/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/16675?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] Enhance /tableConfigs/validate endpoint with cluster-aware validations [pinot]
Posted by "tarun11Mavani (via GitHub)" <gi...@apache.org>.
tarun11Mavani commented on code in PR #16675:
URL: https://github.com/apache/pinot/pull/16675#discussion_r2295460910
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/TableConfigsRestletResource.java:
##########
@@ -210,7 +216,7 @@ public ConfigSuccessResponse addConfig(
}
TableConfigs tableConfigs = tableConfigsAndUnrecognizedProps.getLeft();
String databaseName = DatabaseUtils.extractDatabaseFromHttpHeaders(httpHeaders);
- validateConfig(tableConfigs, databaseName, typesToSkip);
+ validateConfig(tableConfigs, databaseName, SKIP_CLUSTER_VALIDATIONS);
Review Comment:
If we hardcode this, users will not be able to pass typesToSkip. We should avoid this change or have a logic to add SKIP_CLUSTER_VALIDATIONS into typesToSkip before making the call.
IMO we should keep it as is and let it do validation twice.
--
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] Enhance /tableConfigs/validate endpoint with cluster-aware validations [pinot]
Posted by "tarun11Mavani (via GitHub)" <gi...@apache.org>.
tarun11Mavani commented on code in PR #16675:
URL: https://github.com/apache/pinot/pull/16675#discussion_r2295460910
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/TableConfigsRestletResource.java:
##########
@@ -210,7 +216,7 @@ public ConfigSuccessResponse addConfig(
}
TableConfigs tableConfigs = tableConfigsAndUnrecognizedProps.getLeft();
String databaseName = DatabaseUtils.extractDatabaseFromHttpHeaders(httpHeaders);
- validateConfig(tableConfigs, databaseName, typesToSkip);
+ validateConfig(tableConfigs, databaseName, SKIP_CLUSTER_VALIDATIONS);
Review Comment:
If we hardcode this, users will not be able to pass typesToSkip. We should avoid this change or have a logic to add SKIP_CLUSTER_VALIDATIONS into typesToSkip before making the call.
IMO we should keep it as is and let it do validation twice.
--
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] Enhance /tableConfigs/validate endpoint with cluster-aware validations [pinot]
Posted by "noob-se7en (via GitHub)" <gi...@apache.org>.
noob-se7en commented on code in PR #16675:
URL: https://github.com/apache/pinot/pull/16675#discussion_r2345001575
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/TableConfigsRestletResource.java:
##########
@@ -521,6 +532,39 @@ private void validateConfig(TableConfigs tableConfigs, String database, @Nullabl
if (offlineTableConfig != null && realtimeTableConfig != null) {
TableConfigUtils.verifyHybridTableConfigs(rawTableName, offlineTableConfig, realtimeTableConfig);
}
+
+ // Parse validation types to skip (following same pattern as TableConfigUtils.validate)
+ Set<TableConfigUtils.ValidationType> skipTypes = typesToSkip == null ? Collections.emptySet()
+ : Arrays.stream(typesToSkip.split(",")).map(s -> TableConfigUtils.ValidationType.valueOf(s.toUpperCase()))
+ .collect(Collectors.toSet());
Review Comment:
Pls use the TableConfigsUtil method for this, we can make that public.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/TableConfigsRestletResource.java:
##########
@@ -210,7 +216,7 @@ public ConfigSuccessResponse addConfig(
}
TableConfigs tableConfigs = tableConfigsAndUnrecognizedProps.getLeft();
String databaseName = DatabaseUtils.extractDatabaseFromHttpHeaders(httpHeaders);
- validateConfig(tableConfigs, databaseName, typesToSkip);
+ validateConfig(tableConfigs, databaseName, SKIP_CLUSTER_VALIDATIONS);
Review Comment:
+1
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/TableConfigsRestletResource.java:
##########
@@ -106,6 +109,9 @@ public class TableConfigsRestletResource {
public static final Logger LOGGER = LoggerFactory.getLogger(TableConfigsRestletResource.class);
+ // Skip cluster validations during table creation/update since they're performed in the actual table operations
+ private static final String SKIP_CLUSTER_VALIDATIONS = "TENANT,MINION_INSTANCES,ACTIVE_TASKS";
Review Comment:
Pls update API param swagger documentation if including above more types.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/TableConfigsRestletResource.java:
##########
@@ -521,6 +532,39 @@ private void validateConfig(TableConfigs tableConfigs, String database, @Nullabl
if (offlineTableConfig != null && realtimeTableConfig != null) {
TableConfigUtils.verifyHybridTableConfigs(rawTableName, offlineTableConfig, realtimeTableConfig);
}
+
+ // Parse validation types to skip (following same pattern as TableConfigUtils.validate)
+ Set<TableConfigUtils.ValidationType> skipTypes = typesToSkip == null ? Collections.emptySet()
+ : Arrays.stream(typesToSkip.split(",")).map(s -> TableConfigUtils.ValidationType.valueOf(s.toUpperCase()))
+ .collect(Collectors.toSet());
+
+ // Cluster-aware validations
+ if (!skipTypes.contains(TableConfigUtils.ValidationType.ALL)) {
Review Comment:
why not add these new validations in TableConfigUtils.validate method itself?
--
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] Enhance /tableConfigs/validate endpoint with cluster-aware validations [pinot]
Posted by "cbalci (via GitHub)" <gi...@apache.org>.
cbalci merged PR #16675:
URL: https://github.com/apache/pinot/pull/16675
--
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] Enhance /tableConfigs/validate endpoint with cluster-aware validations [pinot]
Posted by "tarun11Mavani (via GitHub)" <gi...@apache.org>.
tarun11Mavani commented on code in PR #16675:
URL: https://github.com/apache/pinot/pull/16675#discussion_r2877430890
##########
.gitignore:
##########
@@ -106,3 +106,6 @@ hf_cache/
# LangChain / LlamaIndex
.langchain/
.llamaindex/
+
+# Claude
Review Comment:
is not intended, let's push this in a separate PR.
--
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] Enhance /tableConfigs/validate endpoint with cluster-aware validations [pinot]
Posted by "jadami10 (via GitHub)" <gi...@apache.org>.
jadami10 commented on PR #16675:
URL: https://github.com/apache/pinot/pull/16675#issuecomment-4791986585
moving here, https://github.com/apache/pinot/pull/18580#issuecomment-4783827290
this ends up causing quite a bit of churn if you have more than 1 pinot cluster as well
1. upgraded clusters will start failing `/tableConfigs/validate` calls if you were just trying to validate table configs without tenants
2. if you try to skip `TENANT`, then this fails on non-upgraded clusters because it can't recognize `TENANT`
--
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