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

Consistency in API response for live broker #12201

Merged
merged 4 commits into from
Mar 26, 2024
Merged

Consistency in API response for live broker #12201

merged 4 commits into from
Mar 26, 2024

Conversation

cypherean
Copy link
Contributor

@cypherean cypherean commented Dec 24, 2023 •
edited
Loading

Resolves #11841

@cypherean cypherean mentioned this pull request Dec 24, 2023
Closed
@cypherean
Copy link
Contributor Author

@Jackie-Jiang comments on linked PR #11850 -

These are not common functions, and I find them quite hard to read. Can we keep it simple? It is okay if we need to repeat some code

done

Query param should be tableName

done

Shall we keep it consistent with other APIs where optional field is represented as @nullable

the default value for optional query params is "", and setting a custom default value also only takes a string

I think we want to put hosts when table exists, even if it is empty

done

This tableName can be raw (without type suffix). The returned map should contain the actual table name with type suffix

done

@codecov-commenter
Copy link

codecov-commenter commented Dec 24, 2023 •
edited
Loading

Codecov Report

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

Project coverage is 34.52%. Comparing base (59551e4) to head (47011bd).
Report is 174 commits behind head on master.

Files Patch % Lines
...ntroller/helix/core/PinotHelixResourceManager.java 0.00% 15 Missing ⚠️
.../controller/api/resources/PinotTableInstances.java 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@              Coverage Diff              @@
##             master   #12201       +/-   ##
=============================================
- Coverage     61.75%   34.52%   -27.24%     
+ Complexity      207        6      -201     
=============================================
  Files          2436     2385       -51     
  Lines        133233   130968     -2265     
  Branches      20636    20292      -344     
=============================================
- Hits          82274    45212    -37062     
- Misses        44911    82432    +37521     
+ Partials       6048     3324     -2724
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 34.48% <0.00%> (-27.23%) ⬇️
java-21 34.40% <0.00%> (-27.23%) ⬇️
skip-bytebuffers-false 34.50% <0.00%> (-27.25%) ⬇️
skip-bytebuffers-true 34.38% <0.00%> (+6.65%) ⬆️
temurin 34.52% <0.00%> (-27.24%) ⬇️
unittests 46.11% <ø> (-15.63%) ⬇️
unittests1 46.11% <ø> (-0.78%) ⬇️
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.

tibrewalpratik17
tibrewalpratik17 reviewed Jan 2, 2024
View reviewed changes
...-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableInstances.java Outdated
try {
return _pinotHelixResourceManager.getTableToLiveBrokersMapping();
return _pinotHelixResourceManager.getTableToLiveBrokersMapping(Optional.of(tableName));
Copy link
Collaborator

@tibrewalpratik17 tibrewalpratik17 Jan 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This might not give Optional.empty() when tableName is empty which should be the desired behaviour here. You can modify it to - tableName.equals("") ? Optional.empty() : Optional.of(tableName) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The check if (optionalTableName.isPresent() && !optionalTableName.get().isEmpty()) in getTableToLiveBrokersMapping checks if the tableName string is empty.
Added a test case for the same.

walterddr
walterddr reviewed Jan 4, 2024
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.

looks good to me overall. comment on the API changes. PTAL

...oller/src/main/java/org/apache/pinot/controller/api/resources/PinotRunningQueryResource.java Outdated Show resolved Hide resolved
...ntroller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java Outdated Show resolved Hide resolved
@cypherean cypherean force-pushed the shreyaa/livebroker branch 2 times, most recently from e52a65b to 28309a6 Compare January 13, 2024 18:12
walterddr
walterddr approved these changes Jan 16, 2024
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.

looks good to me; tagging @Jackie-Jiang to take another look since he originally commented on previous PR

...ntroller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java Outdated
if (nullableTableName != null && !nullableTableName.toString().isEmpty()) {
List<String> tableNameWithType = getExistingTableNamesWithType(nullableTableName.toString(), null);
if (tableNameWithType.isEmpty()) {
throw new TableNotFoundException(String.format("Table=%s not found", nullableTableName));
Copy link
Contributor

Choose a reason for hiding this comment

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

should we make it consistent?

Suggested change
throw new TableNotFoundException(String.format("Table=%s not found", nullableTableName));
throw new ControllerApplicationException(LOGGER, String.format("Table=%s not found", nullableTableName), Response.Status.NOT_FOUND);

@cypherean cypherean force-pushed the shreyaa/livebroker branch 2 times, most recently from ed7a4e8 to 000ba11 Compare February 29, 2024 09:40
@cypherean
Copy link
Contributor Author

@Jackie-Jiang @walterddr please take a look

Jackie-Jiang
Jackie-Jiang reviewed Mar 1, 2024
View reviewed changes
...ntroller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java Outdated
@@ -3921,7 +3921,16 @@ public TableStats getTableStats(String tableNameWithType) {
* Returns map of tableName to list of live brokers
* @return Map of tableName to list of ONLINE brokers serving the table
*/
public Map<String, List<InstanceInfo>> getTableToLiveBrokersMapping() {
public Map<String, List<InstanceInfo>> getTableToLiveBrokersMapping() throws TableNotFoundException {
Copy link
Contributor

Choose a reason for hiding this comment

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

(minor) reformat this change

...ntroller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java Outdated
* Returns map of arg tableName to list of live brokers
* @return Map of arg tableName to list of ONLINE brokers serving the table
*/
public Map<String, List<InstanceInfo>> getTableToLiveBrokersMapping(@Nullable Object nullableTableName)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public Map<String, List<InstanceInfo>> getTableToLiveBrokersMapping(@Nullable Object nullableTableName)
public Map<String, List<InstanceInfo>> getTableToLiveBrokersMapping(@Nullable String tableName)

...ntroller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java Outdated
@@ -3933,6 +3942,29 @@ public Map<String, List<InstanceInfo>> getTableToLiveBrokersMapping() {

Map<String, List<InstanceInfo>> result = new HashMap<>();
ZNRecord znRecord = ev.getRecord();

if (nullableTableName != null && !nullableTableName.toString().isEmpty()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (nullableTableName != null && !nullableTableName.toString().isEmpty()) {
if (StringUtils.isEmpty(tableName)) {

...ntroller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java Outdated
Comment on lines 3953 to 4036
Map<String, String> brokersToState = znRecord.getMapField(tableName);
List<InstanceInfo> hosts = new ArrayList<>();
for (Map.Entry<String, String> brokerEntry : brokersToState.entrySet()) {
if ("ONLINE".equalsIgnoreCase(brokerEntry.getValue())
&& instanceConfigMap.containsKey(brokerEntry.getKey())) {
InstanceConfig instanceConfig = instanceConfigMap.get(brokerEntry.getKey());
hosts.add(new InstanceInfo(instanceConfig.getInstanceName(), instanceConfig.getHostName(),
Integer.parseInt(instanceConfig.getPort())));
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider extracting a common method

...-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableInstances.java Outdated
@@ -165,9 +166,11 @@ public List<String> getLiveBrokersForTable(
@ApiResponses(value = {
@ApiResponse(code = 200, message = "Success"), @ApiResponse(code = 500, message = "Internal server error")
})
public Map<String, List<InstanceInfo>> getLiveBrokers() {
public Map<String, List<InstanceInfo>> getLiveBrokers(
@ApiParam(value = "Table name (with or without type)", required = false) @DefaultValue("")
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we going to ask for liver brokers on multiple tables (e.g. for JOIN queries)? If so, suggest taking tableName as a list. Search for allowMultiple = true to find examples of using list as api param

@cypherean cypherean force-pushed the shreyaa/livebroker branch 2 times, most recently from d283e25 to 781e290 Compare March 18, 2024 05:56
Jackie-Jiang
Jackie-Jiang reviewed Mar 18, 2024
View reviewed changes
...-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableInstances.java Outdated
public Map<String, List<InstanceInfo>> getLiveBrokers(@Context HttpHeaders headers) {
public Map<String, List<InstanceInfo>> getLiveBrokers(@Context HttpHeaders headers,
@ApiParam(value = "Table name list(with or without type)", allowMultiple = true)
@QueryParam("tableNameList") List<String> tableNameList) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest naming the parameter "tables"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Jackie-Jiang addressed

@cypherean cypherean force-pushed the shreyaa/livebroker branch 2 times, most recently from b587d9a to b2e661c Compare March 22, 2024 20:57
@Jackie-Jiang Jackie-Jiang merged commit 2b47523 into apache:master Mar 26, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jackie-Jiang Jackie-Jiang Jackie-Jiang left review comments

@walterddr walterddr walterddr approved these changes

@tibrewalpratik17 tibrewalpratik17 tibrewalpratik17 approved these changes

Assignees
No one assigned
Labels
feature rest-api
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Response parity between Table/getLiveBrokers and Table/getLiveBrokersForTable
5 participants
@cypherean @codecov-commenter @walterddr @Jackie-Jiang @tibrewalpratik17