Skip to content

Add Disabled Table Error#14154

Closed
ashishjayamohan wants to merge 11 commits intoapache:masterfrom
ashishjayamohan:master
Closed

Add Disabled Table Error#14154
ashishjayamohan wants to merge 11 commits intoapache:masterfrom
ashishjayamohan:master

Conversation

@ashishjayamohan
Copy link
Copy Markdown
Contributor

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Oct 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 55.54%. Comparing base (59551e4) to head (e527e23).
Report is 1133 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (59551e4) and HEAD (e527e23). Click for more details.

HEAD has 44 uploads less than BASE
Flag BASE (59551e4) HEAD (e527e23)
integration 7 0
integration2 3 0
temurin 12 3
java-21 7 2
skip-bytebuffers-true 3 1
skip-bytebuffers-false 7 2
unittests 5 3
java-11 5 1
unittests2 3 0
integration1 2 0
custom-integration1 2 0
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #14154      +/-   ##
============================================
- Coverage     61.75%   55.54%   -6.21%     
- Complexity      207      791     +584     
============================================
  Files          2436     2065     -371     
  Lines        133233   108946   -24287     
  Branches      20636    17255    -3381     
============================================
- Hits          82274    60512   -21762     
+ Misses        44911    43607    -1304     
+ Partials       6048     4827    -1221     
Flag Coverage Δ
custom-integration1 ?
integration ?
integration1 ?
integration2 ?
java-11 55.45% <100.00%> (-6.26%) ⬇️
java-21 55.39% <100.00%> (-6.23%) ⬇️
skip-bytebuffers-false 55.49% <100.00%> (-6.26%) ⬇️
skip-bytebuffers-true 55.37% <100.00%> (+27.64%) ⬆️
temurin 55.54% <100.00%> (-6.21%) ⬇️
unittests 55.54% <100.00%> (-6.21%) ⬇️
unittests1 55.54% <100.00%> (+8.65%) ⬆️
unittests2 ?

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

throw QueryException.getException(QueryException.SQL_PARSING_ERROR, ex);
}

String tableName = CalciteSqlParser.compileToPinotQuery(sqlQuery).getDataSource().getTableName();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This will only work if the user issues a query via the controller query API. The recommended way of querying Pinot is via the broker query API (https://docs.pinot.apache.org/users/api/querying-pinot-using-standard-sql) which is also what the controller API delegates to -

private String sendRequestToBroker(String query, String instanceId, String traceEnabled, String queryOptions,

Ideally, we'd want this check in the broker request handler, since all external REST API based queries will always go through there.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@yashmayya Gotcha. I'm looking at the isTableEnabled function in the PinotHelixResourceManager, but can't quite figure out how to use that in the broker request handler. Is there something else I should use instead?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The BaseBrokerStarter which instantiates the single-stage broker request handler and the multi-stage broker request handler also has a HelixAdmin that can be used to check the table's ideal state in ZK (which has the enabled / disabled information). So you could inject this admin (and the cluster name) as a dependency into the broker request handlers from the BaseBrokerStarter and use it to check whether the tables in a request are disabled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants