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.

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

Update the pinot tenants tables api to support returning broker tagged tables #11184

Merged
merged 6 commits into from
Aug 1, 2023
Merged

Update the pinot tenants tables api to support returning broker tagged tables #11184

merged 6 commits into from
Aug 1, 2023

Conversation

jadami10
Copy link
Contributor

@jadami10 jadami10 commented Jul 26, 2023

This is authored by @priyen and is a small feature addition.

This updates the tenants/{tenantName}/tables API to support also specifying type to return tagged tables by the broker tenant. Tables can have separate broker and server tenants, so this allows us to actually get both.

Previously:
/tenants/{tenantName}/tables -> returns tables with that server tag

Now:
/tenants/{tenantName}/tables -> returns tables with that server tag
/tenants/{tenantName}/tables?type=server -> returns tables with that server tag
/tenants/{tenantName}/tables?type=broker -> returns tables with that broker tag

We are already using this successfully in our fork, and unit tests are included

@codecov-commenter
Copy link

codecov-commenter commented Jul 26, 2023

Codecov Report

Merging #11184 (db29dc8) into master (9d7676e) will decrease coverage by 0.01%.
Report is 2 commits behind head on master.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master   #11184      +/-   ##
==========================================
- Coverage    0.11%    0.11%   -0.01%     
==========================================
  Files        2229     2229              
  Lines      119803   119827      +24     
  Branches    18126    18130       +4     
==========================================
  Hits          137      137              
- Misses     119646   119670      +24     
  Partials       20       20              
Flag Coverage Δ
integration1temurin11 0.00% <0.00%> (ø)
integration1temurin17 0.00% <0.00%> (?)
integration1temurin20 0.00% <0.00%> (ø)
integration2temurin11 0.00% <0.00%> (ø)
integration2temurin17 0.00% <0.00%> (ø)
integration2temurin20 0.00% <0.00%> (ø)
unittests1temurin11 0.00% <0.00%> (ø)
unittests1temurin17 0.00% <0.00%> (?)
unittests1temurin20 0.00% <0.00%> (?)
unittests2temurin11 0.11% <0.00%> (-0.01%) ⬇️
unittests2temurin17 0.11% <0.00%> (-0.01%) ⬇️
unittests2temurin20 0.11% <0.00%> (-0.01%) ⬇️

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

Files Changed Coverage Δ
...ller/api/resources/PinotTenantRestletResource.java 0.00% <0.00%> (ø)
...inot/controller/helix/ControllerRequestClient.java 0.00% <0.00%> (ø)
...spi/utils/builder/ControllerRequestURLBuilder.java 0.00% <0.00%> (ø)

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

Copy link
Contributor

@Jackie-Jiang Jackie-Jiang left a comment

Choose a reason for hiding this comment

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

LGTM with minor comments. Please rebase on the latest master since we added the new auth annotation recently

@Jackie-Jiang Jackie-Jiang merged commit 6bce5fa into apache:master Aug 1, 2023
22 checks passed
s0nskar pushed a commit to s0nskar/pinot that referenced this pull request Aug 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants