Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[multistage] Table level Access Validation, QPS Quota, Phase Metrics for multistage queries #10534

Merged
merged 5 commits into from
Apr 10, 2023
Merged

[multistage] Table level Access Validation, QPS Quota, Phase Metrics for multistage queries #10534

merged 5 commits into from
Apr 10, 2023

Conversation

vvivekiyer
Copy link
Contributor

@vvivekiyer vvivekiyer commented Apr 4, 2023 •
edited
Loading

Table Level Access Validation

For non-multistage queries, there are two levels of validations:

  1. Validation with requesterIdentity
  2. Validation with requesterIdentity and BrokerResource (which is typically used to extract table names and perform table level validation)

In MultistageBrokerRequestHandler, we currently only perform (1). This PR adds support for performing table level validations as well.

QPS Quota Checker

Added QPS Quota check for all tables involved in a multistage query

Phase Timers

We have the following phase timers currently:

  • REQUEST_COMPILATION
  • AUTHORIZATION
  • QUERY_ROUTING (This time is included in Compilation when workers are assigned to stages)
  • QUERY_EXECUTION
  • SCATTER_GATHER (We have stage level stats for Multistage)
  • REDUCE (We have stage level stats for Multistage)
  • (Not used) REQUEST_CONNECTION_WAIT
  • (Not used) DESERIALIZATION

Added phase timers for REQUEST_COMPILATION, AUTHORIZATION and QUERY_EXECUTION in this PR. T

cc: @siddharthteotia @somandal @walterddr

@vvivekiyer vvivekiyer changed the title [multistage] Table level ACL for multistage queries [multistage] Table level Access Validation for multistage queries Apr 4, 2023
@codecov-commenter
Copy link

codecov-commenter commented Apr 4, 2023 •
edited
Loading

Codecov Report

Merging #10534 (fb91e84) into master (1fc8150) will increase coverage by 42.49%.
The diff coverage is 51.51%.

@@              Coverage Diff              @@
##             master   #10534       +/-   ##
=============================================
+ Coverage     27.80%   70.29%   +42.49%     
- Complexity       58     6465     +6407     
=============================================
  Files          2087     2103       +16     
  Lines        112307   112838      +531     
  Branches      16918    16992       +74     
=============================================
+ Hits          31226    79323    +48097     
+ Misses        77988    27953    -50035     
- Partials       3093     5562     +2469
Flag Coverage Δ
integration1 24.54% <23.23%> (+0.19%) ⬆️
integration2 24.14% <9.09%> (-0.07%) ⬇️
unittests1 67.84% <91.66%> (?)
unittests2 13.87% <16.16%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ava/org/apache/pinot/broker/api/AccessControl.java 100.00% <ø> (ø)
...broker/broker/ZkBasicAuthAccessControlFactory.java 0.00% <0.00%> (ø)
...requesthandler/MultiStageBrokerRequestHandler.java 62.22% <50.00%> (-5.49%) ⬇️
...t/broker/broker/BasicAuthAccessControlFactory.java 85.71% <85.00%> (-5.20%) ⬇️
.../java/org/apache/pinot/query/QueryEnvironment.java 92.77% <91.66%> (+92.77%) ⬆️
...ot/broker/broker/AllowAllAccessControlFactory.java 100.00% <100.00%> (ø)

... and 1478 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@vvivekiyer vvivekiyer marked this pull request as draft April 4, 2023 17:38
@vvivekiyer vvivekiyer force-pushed the li_table_acl_multistage branch 2 times, most recently from fa2f724 to 90af525 Compare April 4, 2023 19:10
@vvivekiyer vvivekiyer marked this pull request as ready for review April 4, 2023 19:11
@vvivekiyer vvivekiyer changed the title [multistage] Table level Access Validation for multistage queries [multistage] Table level Access Validation, QPS Quota, Phase Metrics for multistage queries Apr 5, 2023
walterddr
walterddr reviewed Apr 6, 2023
View reviewed changes
Copy link
Contributor

@walterddr walterddr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm mostly. thank you for adding the tests

pinot-broker/src/main/java/org/apache/pinot/broker/broker/ZkBasicAuthAccessControlFactory.java Outdated Show resolved Hide resolved
...ker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java Outdated Show resolved Hide resolved
jackjlli
jackjlli reviewed Apr 6, 2023
View reviewed changes
...ker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java Outdated Show resolved Hide resolved
...ker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java
@@ -155,11 +160,22 @@ private BrokerResponse handleRequest(long requestId, String query,
compilationStartTimeNs = System.nanoTime();
switch (sqlNodeAndOptions.getSqlNode().getKind()) {
case EXPLAIN:
String plan = _queryEnvironment.explainQuery(query, sqlNodeAndOptions);
queryPlanResult = _queryEnvironment.explainQuery(query, sqlNodeAndOptions);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For EXPLAIN cases, shouldn't we consider deducting the qps quota for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessarily for now because we don't send explain plan queries to server.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have the plan to support sending EXPLAIN queries to servers? If yes, probably we can add a TODO here in case if it's forgotten?

Copy link
Contributor

@walterddr walterddr Apr 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's no reason to send EXPLAIN PLAN to server since broker has all the information the servers have in order to create the PLAN. the only information on server is whether segment A has inverted index; and segment B doesn't have it (e.g. it was generated with a different table config). and

  1. sending per-segment level physical plan info back to user is neither necessary nor practical
  2. we can always view segment level info via other APIs

@Jackie-Jiang Jackie-Jiang added feature security multi-stage Related to the multi-stage query engine release-notes Referenced by PRs that need attention when compiling the next release notes labels Apr 10, 2023
@siddharthteotia siddharthteotia merged commit 70c4c5b into apache:master Apr 10, 2023
@vvivekiyer vvivekiyer deleted the li_table_acl_multistage branch June 7, 2023 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@walterddr walterddr walterddr approved these changes

@jackjlli jackjlli jackjlli left review comments

@somandal somandal somandal approved these changes

@siddharthteotia siddharthteotia siddharthteotia approved these changes

Assignees
No one assigned
Labels
feature multi-stage Related to the multi-stage query engine release-notes Referenced by PRs that need attention when compiling the next release notes security
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@vvivekiyer @codecov-commenter @siddharthteotia @walterddr @jackjlli @somandal @Jackie-Jiang