Add Server API to get tenant pools #11273
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MeihanLi
force-pushed
the
server_pool_api
branch
2 times, most recently
from
4330ccb
to
e390e53
Compare
|
cc: @chenboat can you help review it? Thanks! |
Codecov Report
@@ Coverage Diff @@
## master #11273 +/- ##
==========================================
- Coverage 0.11% 0.11% -0.01%
==========================================
Files 2229 2239 +10
Lines 119963 120429 +466
Branches 18183 18217 +34
==========================================
Hits 137 137
- Misses 119806 120272 +466
Partials 20 20
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 58 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
ankitsultana
reviewed
Aug 7, 2023
| }) | ||
| @Produces(MediaType.APPLICATION_JSON) | ||
| public Map<String, String> getInstancePools() { | ||
| InstanceConfig instanceConfig = HelixHelper.getInstanceConfig(_helixManager, _instanceId); |
There was a problem hiding this comment.
need null check for instanceConfig (similar to getInstanceTags)
There was a problem hiding this comment.
Thanks Ankit, addressed
MeihanLi
force-pushed
the
server_pool_api
branch
from
e390e53
to
798da32
Compare
chenboat
reviewed
Aug 11, 2023
| @ApiOperation(value = "Tenant pools for current instance") | ||
| @ApiResponses(value = { | ||
| @ApiResponse(code = 200, message = "Success"), @ApiResponse(code = 500, message = "Internal server error") | ||
| }) |
There was a problem hiding this comment.
- can you add the javadoc to explain return result types? i.e.,The summary in the PR looks good.
- Can you add a Pinot doc reference to the definition of pool in Pinot?
chenboat
approved these changes
Aug 11, 2023
MeihanLi
force-pushed
the
server_pool_api
branch
from
302cfae
to
a63fb4d
Compare
ankitsultana
approved these changes
Aug 11, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add a new Server API GET /instance/pools in InstanceResource to get server pools.
featureIt returns an empty map if the server is not assigned to any pool number
Tested locally: