Allow passing database context through `database` http header by shounakmk219 · Pull Request #12417 · apache/pinot

shounakmk219 · 2024-02-14T07:19:04Z

Description

This PR aims to allow a way of passing the database context along with the API requests without disrupting the existing APIs.
The database context will be passed as a database http header along with the API requests. It is part of the effort to support database concept in Pinot #12333

Release Notes

Allow passing database name context through database http header

- Allow passing database through http header - Create v2 endpoints for ones which accept table name as path param or part of request body.

codecov-commenter · 2024-02-14T07:54:59Z

Codecov Report

Attention: Patch coverage is 55.09554% with 141 lines in your changes are missing coverage. Please review.

Project coverage is 61.65%. Comparing base (59551e4) to head (a7d6f93).
Report is 121 commits behind head on master.

Files	Patch %	Lines
...ntroller/helix/core/PinotHelixResourceManager.java	29.41%	17 Missing and 7 partials ⚠️
...oller/api/resources/PinotTableRestletResource.java	66.66%	14 Missing and 8 partials ⚠️
...ler/api/resources/PinotSegmentRestletResource.java	25.92%	20 Missing ⚠️
...roller/api/resources/PinotTaskRestletResource.java	0.00%	9 Missing ⚠️
...ler/api/resources/TableConfigsRestletResource.java	75.86%	2 Missing and 5 partials ⚠️
...java/org/apache/pinot/spi/config/TableConfigs.java	0.00%	7 Missing ⚠️
...a/org/apache/pinot/common/utils/DatabaseUtils.java	68.75%	3 Missing and 2 partials ⚠️
...ller/api/resources/PinotRealtimeTableResource.java	0.00%	5 Missing ⚠️
...ller/api/resources/PinotSchemaRestletResource.java	87.80%	1 Missing and 4 partials ⚠️
...ller/api/resources/PinotBrokerRestletResource.java	55.55%	4 Missing ⚠️
... and 14 more

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #12417      +/-   ##
============================================
- Coverage     61.75%   61.65%   -0.10%     
+ Complexity      207      198       -9     
============================================
  Files          2436     2455      +19     
  Lines        133233   134004     +771     
  Branches      20636    20781     +145     
============================================
+ Hits          82274    82623     +349     
- Misses        44911    45246     +335     
- Partials       6048     6135      +87

Flag	Coverage Δ
custom-integration1	`<0.01% <0.00%> (-0.01%)`	⬇️
integration	`<0.01% <0.00%> (-0.01%)`	⬇️
integration1	`<0.01% <0.00%> (-0.01%)`	⬇️
integration2	`0.00% <0.00%> (ø)`
java-11	`<0.01% <0.00%> (-61.71%)`	⬇️
java-21	`61.65% <55.09%> (+0.03%)`	⬆️
skip-bytebuffers-false	`61.65% <55.09%> (-0.10%)`	⬇️
skip-bytebuffers-true	`0.00% <0.00%> (-27.73%)`	⬇️
temurin	`61.65% <55.09%> (-0.10%)`	⬇️
unittests	`61.65% <55.09%> (-0.10%)`	⬇️
unittests1	`46.75% <44.44%> (-0.14%)`	⬇️
unittests2	`27.70% <51.27%> (-0.04%)`	⬇️

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.

Jackie-Jiang · 2024-02-14T18:55:00Z

cc @zhtaoxiang to also take a look

pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/BaseBrokerStarter.java

pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java

pinot-common/src/main/java/org/apache/pinot/common/utils/DatabaseUtils.java

Jackie-Jiang

Can we also process the path parameter during the request filtering? IIRC there is a filter that can be applied after matching the path, and I don't like the idea of adding all these v2 APIs

pinot-common/src/main/java/org/apache/pinot/common/utils/DatabaseUtils.java

shounakmk219 · 2024-02-28T06:56:05Z

Can we also process the path parameter during the request filtering? IIRC there is a filter that can be applied after matching the path, and I don't like the idea of adding all these v2 APIs

Hey @Jackie-Jiang we can't update the URI in post match phase and path param context is only available there (doc ref).
Due to this limitation we are forced to manually handle the translation for the endpoints having table name as path param. Intent of the v2 APIs is to discourage the table name input as path param and eventually deprecate it so that we have a centralised place where translation is performed and avoid the maintenance overhead around it.
Let me know your thoughts.

Jackie-Jiang

I don't think we need to define a "default" database. If database is provided, then it is not default; if it is not provided, we should directly use table name as is

pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java

pinot-common/src/main/java/org/apache/pinot/common/config/provider/TableCache.java

pinot-common/src/main/java/org/apache/pinot/common/utils/DatabaseUtils.java

…table name values with right header

pinot-common/src/main/java/org/apache/pinot/common/config/provider/TableCache.java

pinot-common/src/main/java/org/apache/pinot/common/utils/DatabaseUtils.java

...ntroller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java

shounakmk219 · 2024-03-12T09:26:54Z

...ntroller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java

-    if (schemas != null) {
-        return schemas.stream().filter(schemaName -> isPartOfDatabase(schemaName, databaseName))
-            .collect(Collectors.toList());
+    if (databaseName != null) {


This will return all schemas when databaseName is null, shouldn't the null database name be considered as the default database and only return schemas under the default database?

Good call. We need to loop over all schemas

shounakmk219 · 2024-03-12T09:31:47Z

...ler/src/main/java/org/apache/pinot/controller/api/resources/TableConfigsRestletResource.java

-      Preconditions.checkState(
-          DatabaseUtils.translateTableName(tableConfigs.getTableName(), headers).equals(translatedTableName),
-          "Table name mismatch: %s is not equivalent to %s", tableConfigs.getTableName(), tableName);
+      tableConfigs.setTableName(DatabaseUtils.translateTableName(tableConfigs.getTableName(), headers));


setting the table name here will make all the validations on table/schema names redundant which are part of validateConfig(tableConfigs, typesToSkip); called below. This in a way will allow user to pass invalid schema or realtime/offline table names as ultimately it gets overridden by the translated raw table name.

Good point. Changed the order

npawar · 2024-05-22T20:11:44Z

@shounakmk219 do we have documentation on this feature?

shounakmk219 added 4 commits February 13, 2024 11:31

Request filters to translate table name

a169915

Make endpoints database aware

b7b93d8

- Allow passing database through http header - Create v2 endpoints for ones which accept table name as path param or part of request body.

fix column name extraction logic when prefixed with database

3a1ba75

test and checkstyle fixes

a29f5f5

shounakmk219 marked this pull request as draft February 14, 2024 07:19

test fixes

7c8ba97

shounakmk219 mentioned this pull request Feb 14, 2024

Add Database support in Pinot #12333

Open

4 tasks

shounakmk219 marked this pull request as ready for review February 14, 2024 12:50

Merge remote-tracking branch 'upstream/master' into db-request-filter

96fd366

Jackie-Jiang added feature release-notes Referenced by PRs that need attention when compiling the next release notes labels Feb 14, 2024

Jackie-Jiang requested review from npawar, saurabhd336 and xiangfu0 February 14, 2024 18:53

xiangfu0 reviewed Feb 19, 2024

View reviewed changes

pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/BaseBrokerStarter.java Outdated Show resolved Hide resolved

shounakmk219 added 5 commits February 21, 2024 16:36

Handle deprecated config segmentsConfig.schemaName to avoid false errors

09e129b

Move tableCache registration to start()

b5e14c2

fix false errors upon mixed table name formats being passed

5f28a09

Expose list database controller API

bb4ef42

Merge remote-tracking branch 'upstream/master' into db-request-filter

b7c94a1

klsince reviewed Feb 26, 2024

View reviewed changes

shounakmk219 added 3 commits February 27, 2024 19:43

refactor getActualColumnName to cover all edge cases

37c75e8

Merge remote-tracking branch 'upstream/master' into db-request-filter

872d0ac

handle null names for equivalence

dd7a62c

Jackie-Jiang reviewed Feb 27, 2024

View reviewed changes

shounakmk219 added 2 commits February 28, 2024 19:03

add java doc

b8d1f43

set uri outside the loop

6144ab0

shounakmk219 added 5 commits March 5, 2024 02:38

Merge remote-tracking branch 'upstream/master' into db-request-filter

8e4e089

checkstyle fix

5b9f014

cleanup

f7971f3

Bugfix. Handle default db header for get table flows

9540b4b

Merge remote-tracking branch 'upstream/master' into db-request-filter

1a75690

Jackie-Jiang reviewed Mar 6, 2024

View reviewed changes

shounakmk219 added 4 commits March 6, 2024 17:12

API payloads should either have all translated values or all logical …

37362e3

…table name values with right header

refactors and minor fixes

fd1e750

Merge remote-tracking branch 'upstream/master' into db-request-filter

e9f8f7a

add missing table name translations on query params

12bca07

Jackie-Jiang reviewed Mar 7, 2024

View reviewed changes

shounakmk219 and others added 6 commits March 8, 2024 23:03

revert adding entries for default.tableName in tableCache name maps

f54dee4

fixes and reformats

f5f7b30

Merge remote-tracking branch 'upstream/master' into db-request-filter

6a417b9

remove redundant default database check

73621f9

Merge remote-tracking branch 'upstream/master' into db-request-filter

1b21160

Misc fixes: ignore case, table config and schema handling

6f6401b

shounakmk219 commented Mar 12, 2024

View reviewed changes

shounakmk219 and others added 2 commits March 13, 2024 00:04

ensure translated name is sent to auth validations

68d17ee

Fix test and fine grained access control

a7d6f93

Jackie-Jiang approved these changes Mar 12, 2024

View reviewed changes

Jackie-Jiang merged commit b06c061 into apache:master Mar 12, 2024

shounakmk219 mentioned this pull request Mar 15, 2024

[multistage] support database in v2 #12591

Merged

This was referenced Mar 22, 2024

Add a way to pass database context as header from swagger UI #12695

Merged

Bugfix. Avoid passing null table name input to translation util #12726

Merged

jtao15 mentioned this pull request Mar 29, 2024

NPE when broker header is null #12745

Closed

jackjlli mentioned this pull request Apr 2, 2024

Bug fix. Allow passing null http headers object to translateTableName #12764

Merged

shounakmk219 mentioned this pull request Apr 4, 2024

Handle table name translation on missed api resources #12792

Merged

Conversation

shounakmk219 commented Feb 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Release Notes

Uh oh!

codecov-commenter commented Feb 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Jackie-Jiang commented Feb 14, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shounakmk219 commented Feb 28, 2024

Uh oh!

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shounakmk219 Mar 12, 2024

Choose a reason for hiding this comment

Uh oh!

Jackie-Jiang Mar 12, 2024

Choose a reason for hiding this comment

Uh oh!

shounakmk219 Mar 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jackie-Jiang Mar 12, 2024

Choose a reason for hiding this comment

Uh oh!

npawar commented May 22, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

shounakmk219 commented Feb 14, 2024 •

edited

Loading

codecov-commenter commented Feb 14, 2024 •

edited

Loading

shounakmk219 Mar 12, 2024 •

edited

Loading