You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "Yuyaoo (via GitHub)" <gi...@apache.org> on 2025/03/21 20:36:50 UTC
[PR] Revert "Consolidate Auth and Validation in SSE Request Handler" [pinot]
Yuyaoo opened a new pull request, #15346:
URL: https://github.com/apache/pinot/pull/15346
PR apache/pinot#15240 introduced a backwards-incompatible change that has broken our build. Specifically, it removed the use of `accessControl.authorize` with the `brokerRequest` ([previous usage](https://github.com/apache/pinot/pull/15240/files#diff-06e2bf5ea89ac7b1f67fa90f3d3e9bbb8c61cae03c20093fd18bdc977f949134L381)). Our system relies heavily on this `brokerRequest` to perform some query analysis and parsing, making this removal problematic
This PR temporarily revert the change to restore compatibility while we explore a long-term solution, for example exposing a separate function to pass the `brokerRequest`.
--
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] Revert "Consolidate Auth and Validation in SSE Request Handler" [pinot]
Posted by "vrajat (via GitHub)" <gi...@apache.org>.
vrajat commented on PR #15346:
URL: https://github.com/apache/pinot/pull/15346#issuecomment-2745032294
I re-read the code to see if the call to broker request can be added on instead of reverting the commit. The only other option I see is :
* Add the function to authorize using broker request.
* Remove the table check from `authorize(..., BrokerRequest ...)` e.g. https://github.com/apache/pinot/blob/master/pinot-broker/src/main/java/org/apache/pinot/broker/broker/ZkBasicAuthAccessControlFactory.java#L100
--
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] Revert "Consolidate Auth and Validation in SSE Request Handler" [pinot]
Posted by "vrajat (via GitHub)" <gi...@apache.org>.
vrajat commented on PR #15346:
URL: https://github.com/apache/pinot/pull/15346#issuecomment-2745029954
> Is there a new function that super-sedes this deprecated function and does have the BrokerRequest in the method signature ?
>
> I understand something has been deprecated but ideally the PR that deprecated that API should have also added a newer version of the API that existing users who depended on the deprecated API can move to the new one and continue getting the prior behavior ?
The function is not deprecated. Both the implementation in this repo extract the table name from the broker request. Also MSE doesnt check BrokerRequest. This is why we assumed it is OK to consolidate. Also the previous implementation wasnt correct wrt to phase time metrics.
--
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] Revert "Consolidate Auth and Validation in SSE Request Handler" [pinot]
Posted by "siddharthteotia (via GitHub)" <gi...@apache.org>.
siddharthteotia commented on PR #15346:
URL: https://github.com/apache/pinot/pull/15346#issuecomment-2751821054
Thanks for the prompt help @vrajat @gortiz
What is the next step on this in terms of taking it forward for what you originally wanted to do ? Is there a new PR / issue where you are following-up to address this properly ?
--
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] Revert "Consolidate Auth and Validation in SSE Request Handler" [pinot]
Posted by "siddharthteotia (via GitHub)" <gi...@apache.org>.
siddharthteotia commented on PR #15346:
URL: https://github.com/apache/pinot/pull/15346#issuecomment-2744735939
Can we merge this please ?
--
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] Revert "Consolidate Auth and Validation in SSE Request Handler" [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #15346:
URL: https://github.com/apache/pinot/pull/15346#issuecomment-2744466800
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/15346?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 56.00%. 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 [(`afc642d`)](https://app.codecov.io/gh/apache/pinot/commit/afc642d70809f94082a97451fe1897d5487168a2?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 1891 commits behind head on master.
> :exclamation: There is a different number of reports uploaded between BASE (59551e4) and HEAD (afc642d). Click for more details.
>
> <details><summary>HEAD has 48 uploads less than BASE</summary>
>
>| Flag | BASE (59551e4) | HEAD (afc642d) |
>|------|------|------|
>|integration|7|0|
>|integration2|3|0|
>|temurin|12|2|
>|java-21|7|2|
>|skip-bytebuffers-true|3|1|
>|skip-bytebuffers-false|7|1|
>|unittests|5|2|
>|java-11|5|0|
>|unittests2|3|0|
>|integration1|2|0|
>|custom-integration1|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #15346 +/- ##
============================================
- Coverage 61.75% 56.00% -5.75%
- Complexity 207 685 +478
============================================
Files 2436 2174 -262
Lines 133233 117275 -15958
Branches 20636 18750 -1886
============================================
- Hits 82274 65678 -16596
- Misses 44911 46200 +1289
+ Partials 6048 5397 -651
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/15346/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/15346/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/15346/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/15346/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/15346/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/15346/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/15346/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.00% <ø> (-5.63%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/15346/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.93% <ø> (-5.82%)` | :arrow_down: |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/15346/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.96% <ø> (+28.24%)` | :arrow_up: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/15346/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.00% <ø> (-5.75%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/15346/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.00% <ø> (-5.75%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/15346/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.00% <ø> (+9.11%)` | :arrow_up: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/15346/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/15346?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>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :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] Revert "Consolidate Auth and Validation in SSE Request Handler" [pinot]
Posted by "vrajat (via GitHub)" <gi...@apache.org>.
vrajat commented on PR #15346:
URL: https://github.com/apache/pinot/pull/15346#issuecomment-2753325161
I've filed https://github.com/apache/pinot/issues/15373 Lets discuss in that issue ?
--
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] Revert "Consolidate Auth and Validation in SSE Request Handler" [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #15346:
URL: https://github.com/apache/pinot/pull/15346#issuecomment-2744437818
cc @vrajat @gortiz
--
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] Revert "Consolidate Auth and Validation in SSE Request Handler" [pinot]
Posted by "siddharthteotia (via GitHub)" <gi...@apache.org>.
siddharthteotia commented on PR #15346:
URL: https://github.com/apache/pinot/pull/15346#issuecomment-2744736110
@vrajat @gortiz - Appreciate your help here !!
--
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] Revert "Consolidate Auth and Validation in SSE Request Handler" [pinot]
Posted by "vrajat (via GitHub)" <gi...@apache.org>.
vrajat commented on PR #15346:
URL: https://github.com/apache/pinot/pull/15346#issuecomment-2746955067
A draft PR of adding an authorize step on the broker as well as removing table checks from that step.
https://github.com/apache/pinot/pull/15351
--
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] Revert "Consolidate Auth and Validation in SSE Request Handler" [pinot]
Posted by "siddharthteotia (via GitHub)" <gi...@apache.org>.
siddharthteotia commented on PR #15346:
URL: https://github.com/apache/pinot/pull/15346#issuecomment-2744408922
Is there a new function in that super-sedes this deprecated function and does have the BrokerRequest in the method signature ?
I understand something has been deprecated but shouldn't we have a newer version of the API that existing users who depended on the deprecated API can move to the new one and continue getting the prior behavior ?
--
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] Revert "Consolidate Auth and Validation in SSE Request Handler" [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on PR #15346:
URL: https://github.com/apache/pinot/pull/15346#issuecomment-2747876372
> Specifically, it removed the use of accessControl.authorize with the brokerRequest parameter ([previous usage](https://github.com/apache/pinot/pull/15240/files#diff-06e2bf5ea89ac7b1f67fa90f3d3e9bbb8c61cae03c20093fd18bdc977f949134L381)). Our system relies heavily on this brokerRequest to perform some query analysis and parsing, making this removal problematic
--
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] Revert "Consolidate Auth and Validation in SSE Request Handler" [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz merged PR #15346:
URL: https://github.com/apache/pinot/pull/15346
--
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