Add Support for Getting Live Brokers for a Table (without type suffix) by ankitsultana · Pull Request #8188 · apache/pinot

ankitsultana · 2022-02-10T08:35:50Z

Description

At present the /tables/{tableNameWithType}/livebrokers API doesn't support table names without the type suffix. Upstream callers that are not aware of the available types for a given table will have to try the API for both the table-types to determine the available brokers.

We can either update the existing API to handle tables names without type or add a new API. Updating the existing API would inevitably change the behavior of the API so it can cause compatibility issues.

This was also discussed in the PR for adding the livebrokers API as a follow-up: e2e3aeb#r738959901

Test Plan: I have added a UT. Also tested on a local Pinot instance. Output below:

┌  /tmp
❯❯❯ curl -X GET "http://localhost:9000/tables/baseballStats/livebrokers" -H "accept: application/json"
["Broker_127.0.0.1_8000"]%                                                                                                     
┌  /tmp
❯❯❯ curl -X GET "http://localhost:9000/tables/baseballStats_OFFLINE/livebrokers" -H "accept: application/json"
["Broker_127.0.0.1_8000"]%                                                                                                     
┌  /tmp
❯❯❯ curl -X GET "http://localhost:9000/tables/baseballStats_REALTIME/livebrokers" -H "accept: application/json"
{"code":404,"error":"Table=baseballStats_REALTIME not found"}%                                                                 
┌  /tmp
❯❯❯ curl -X GET "http://localhost:9000/tables/baseballStats_sdfs/livebrokers" -H "accept: application/json"
{"code":404,"error":"Table=baseballStats_sdfs not found"}%                                                                     
┌  /tmp
❯❯❯

Upgrade Notes

Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)

No

Does this PR fix a zero-downtime upgrade introduced earlier?

No

Does this PR otherwise need attention when creating release notes? Things to consider:

New configuration options
Deprecation of configurations
Signature changes to public methods/interfaces
New plugins added or old plugins removed

Yes (Please label this PR as release-notes and complete the section on Release Notes)

Release Notes

Documentation

mcvsubbu · 2022-02-10T17:01:04Z

Adding more APIs makes us pay more tech debt in terms of backward compatibility, etc. We do have a list tables by type API, can u consider using that? We shoul not be adding new APIs to resolve a minor inconvenience, IMO.

ankitsultana · 2022-02-10T18:59:24Z

@mcvsubbu : Our main use case is to get live brokers of a table. For upstream callers like say Presto, users and the caller aren't aware of the different table-types associated with a given table, and hence the only way to get live brokers for a table in that case would be to try the current API with both table types. Lmk if there's another way to do this.

I looked around and it seems this is the only /tables API which uses tableNameWithType, all the other APIs use tableName, so I think moving to this API would make things more consistent.

I agree adding APIs isn't tenable and prone to adding more tech debt, but I think in this case this is also cleaning up older tech debt (this was also a comment on the original PR: e2e3aeb#r738959901).

I think there are a couple other options worth considering:

Making this change within the existing API. This would change the behavior of the API only if the passed table name doesn't contain table type. Still, this is a change in behavior so not exactly backwards compatible.
Deprecate the previous /tables/{tableNameWithType}/livebrokers API and keep this one. This will make the APIs consistent in that they all take in a table name either with or without type.

Jackie-Jiang · 2022-02-10T21:16:40Z

Let's change the existing API to take table name (with or without type suffix).

If type suffix is provided, we follow the existing behavior
If type suffix is not provided, check the table existence. If only one of OFFLINE/REALTIME table exists, follow the existing behavior; if both table exists, return the intersection of the hosts

ankitsultana · 2022-02-12T02:56:28Z

@Jackie-Jiang : Thanks. I have updated the PR.

Note: At present the API doesn't return a 404 in case the table doesn't exist. It instead returns an empty list and hence I am also returning the same. Should we update the docs too accordingly? (remove @ApiReponse(code = 404... line)

ankitsultana · 2022-02-14T22:37:42Z

Thanks @chenboat for the review.

@Jackie-Jiang can you also take a look?

Jackie-Jiang

I'd suggest checking the table existence and throw exception if table does not exist to be consistent with other APIs. Within PinotHelixResourceManager, you may throw TableNotFoundException to pass this information

Jackie-Jiang · 2022-02-15T22:34:44Z

...-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableInstances.java

Please set up the Pinot Style and re-format the code

Ah good catch.. didn't know about this. Fixed

...-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableInstances.java

codecov-commenter · 2022-02-18T01:08:04Z

Codecov Report

Merging #8188 (5a9664e) into master (582d621) will increase coverage by 5.37%.
The diff coverage is 52.17%.

@@             Coverage Diff              @@
##             master    #8188      +/-   ##
============================================
+ Coverage     64.28%   69.66%   +5.37%     
+ Complexity     4312     4239      -73     
============================================
  Files          1581     1629      +48     
  Lines         83122    85159    +2037     
  Branches      12587    12820     +233     
============================================
+ Hits          53436    59326    +5890     
+ Misses        25852    21698    -4154     
- Partials       3834     4135     +301

Flag	Coverage Δ
integration1	`28.78% <8.69%> (?)`
unittests1	`66.93% <ø> (-0.42%)`	⬇️
unittests2	`14.10% <52.17%> (+0.01%)`	⬆️

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

Impacted Files	Coverage Δ
.../controller/api/resources/PinotTableInstances.java	`0.00% <0.00%> (ø)`
...ntroller/helix/core/PinotHelixResourceManager.java	`65.92% <63.15%> (+3.87%)`	⬆️
...ls/nativefst/automaton/MinimizationOperations.java	`0.00% <0.00%> (-36.20%)`	⬇️
...tils/nativefst/automaton/TransitionComparator.java	`9.37% <0.00%> (-25.00%)`	⬇️
...ent/local/utils/nativefst/automaton/Automaton.java	`57.08% <0.00%> (-16.86%)`	⬇️
...gment/spi/partition/HashCodePartitionFunction.java	`66.66% <0.00%> (-11.12%)`	⬇️
...cal/utils/nativefst/automaton/BasicOperations.java	`28.57% <0.00%> (-10.85%)`	⬇️
...ore/query/scheduler/resources/ResourceManager.java	`85.71% <0.00%> (-10.72%)`	⬇️
...segment/local/utils/nativefst/automaton/State.java	`36.58% <0.00%> (-9.76%)`	⬇️
.../pinot/core/query/scheduler/PriorityScheduler.java	`78.08% <0.00%> (-5.48%)`	⬇️
... and 365 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 582d621...5a9664e. Read the comment docs.

Jackie-Jiang

The overall logic looks good

Jackie-Jiang · 2022-02-18T19:15:21Z

...-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableInstances.java

  @ApiResponses(value = {
-      @ApiResponse(code = 200, message = "Success"),
-      @ApiResponse(code = 404, message = "Table not found"),
+      @ApiResponse(code = 200, message = "Success"), @ApiResponse(code = 404, message = "Table not found"),


revert this

Jackie-Jiang · 2022-02-18T19:19:21Z

...-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableInstances.java

+    try {
+      return _pinotHelixResourceManager.getLiveBrokersForTable(tableName);
+    } catch (TableNotFoundException tableNotFoundException) {
+      throw new WebApplicationException(String.format("Table=%s not found", tableName), 404);


We have some example of returning table not found in PinotTableRestletResource

Suggested change

throw new WebApplicationException(String.format("Table=%s not found", tableName), 404);

throw new ControllerApplicationException(LOGGER, String.format("Table '%s' does not exist", tableName),

Response.Status.NOT_FOUND);

Jackie-Jiang · 2022-02-18T19:23:29Z

...-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableInstances.java

+    }
+  }
+
+  public void setPinotHelixResourceManager(PinotHelixResourceManager pinotHelixResourceManager) {


Suggest removing this function and PinotTableInstancesTest. The test in PinotHelixResourceManagerTest is good enough, no need to mock a PinotHelixResourceManager for one line of code

Jackie-Jiang · 2022-02-18T19:29:46Z

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

   *  Broker_hostname_port
   */
-  public List<String> getLiveBrokersForTable(String tableNameWithType) {
+  public List<String> getLiveBrokersForTable(String tableName, TableType tableType) {


Suggest merging the logic of this method into the main method to avoid looking up BROKER_RESOURCE twice. Also (not introduced in this PR), when BROKER_RESOURCE cannot be found, we should probably throw an exception instead of returning empty result

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

Jackie-Jiang · 2022-02-18T19:33:17Z

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

+      }
+      return getLiveBrokersForTable(tableName, inputTableType);
+    }
+    boolean hasOfflineTable = hasOfflineTable(tableName);


(minor) Generate offlineTableName and realtimeTableName using TableNameBuilder.OFFLINE.tableNameWithType(tableName) and TableNameBuilder.REALTIME.tableNameWithType(tableName) which can be used to check table existence and lookup the external view to avoid generating them multiple times.

...ller/src/test/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManagerTest.java

ankitsultana · 2022-02-22T06:17:38Z

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

    ExternalView ev = _helixDataAccessor.getProperty(_keyBuilder.externalView(Helix.BROKER_RESOURCE_INSTANCE));
    if (ev == null) {
-      return Collections.EMPTY_LIST;
+      return new ArrayList<>();


@Jackie-Jiang : Can you specify which exception should we return when ExternalView is null?

We can throw new IllegalStateException("Failed to find external view for " + Helix.BROKER_RESOURCE_INSTANCE), and the rest API should return 500

Jackie-Jiang

LGTM with minor comments

Jackie-Jiang · 2022-02-22T22:14:50Z

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

    ExternalView ev = _helixDataAccessor.getProperty(_keyBuilder.externalView(Helix.BROKER_RESOURCE_INSTANCE));
    if (ev == null) {
-      return Collections.EMPTY_LIST;
+      return new ArrayList<>();


We can throw new IllegalStateException("Failed to find external view for " + Helix.BROKER_RESOURCE_INSTANCE), and the rest API should return 500

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

...ller/src/test/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManagerTest.java

chenboat · 2022-02-23T16:54:21Z

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

+   * 3. If the tableName doesn't have a type-suffix and there are both REALTIME
+   *    and OFFLINE tables, then the intersection of the brokers for the two table-types
+   *    would be returned. Intersection is taken since the method guarantees to return
+   *    brokers which can serve the given table. In case of no type provided, it returns


"In case of no type provided..." -- isn't this sentence a repeat of what is stated right before it?

ankitsultana changed the title ~~Add Support for Getting Live Brokers for a Table (without type suffix)~~ Fixes 8187: Add Support for Getting Live Brokers for a Table (without type suffix) Feb 10, 2022

ankitsultana changed the title ~~Fixes 8187: Add Support for Getting Live Brokers for a Table (without type suffix)~~ fixes 8187: Add Support for Getting Live Brokers for a Table (without type suffix) Feb 10, 2022

ankitsultana changed the title ~~fixes 8187: Add Support for Getting Live Brokers for a Table (without type suffix)~~ Add Support for Getting Live Brokers for a Table (without type suffix) Feb 10, 2022

chenboat approved these changes Feb 14, 2022

View reviewed changes

ankitsultana force-pushed the livebroker-by-type branch from dceb3ca to 31f17a6 Compare February 14, 2022 21:19

Jackie-Jiang reviewed Feb 15, 2022

View reviewed changes

Ankit Sultana added 5 commits February 17, 2022 17:46

Add Support for Getting Live Brokers for a Table (without type suffix)

2806b70

Fix formatting

3399bcf

Address feedback; merge changes with existing API

f169ef7

Reformat using pinot style

3a6a426

Address feedback.. test on local

f29feed

ankitsultana force-pushed the livebroker-by-type branch from 9104dbe to f29feed Compare February 18, 2022 01:47

Jackie-Jiang reviewed Feb 18, 2022

View reviewed changes

Address feedback

b59648e

ankitsultana commented Feb 22, 2022

View reviewed changes

ankitsultana requested a review from Jackie-Jiang February 22, 2022 06:18

Jackie-Jiang approved these changes Feb 22, 2022

View reviewed changes

Reformat files

e9b83b7

chenboat reviewed Feb 23, 2022

View reviewed changes

Fix comment

5a9664e

chenboat merged commit 9536aa5 into apache:master Feb 24, 2022

	throw new WebApplicationException(String.format("Table=%s not found", tableName), 404);
	throw new ControllerApplicationException(LOGGER, String.format("Table '%s' does not exist", tableName),
	Response.Status.NOT_FOUND);

Conversation

ankitsultana commented Feb 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Upgrade Notes

Release Notes

Documentation

Uh oh!

mcvsubbu commented Feb 10, 2022

Uh oh!

ankitsultana commented Feb 10, 2022

Uh oh!

Jackie-Jiang commented Feb 10, 2022

Uh oh!

ankitsultana commented Feb 12, 2022

Uh oh!

ankitsultana commented Feb 14, 2022

Uh oh!

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ankitsultana commented Feb 10, 2022 •

edited

Loading

codecov-commenter commented Feb 18, 2022 •

edited

Loading