Let the controller getTableInstance() call to return the list of live brokers of a table. #7556

chenboat · 2021-10-11T19:19:51Z

Description

Fix the issue described in #7491. Today the controller getTableInstance api gets the brokers serving a table based on the table's broker tenant config. It fetches the brokers list based only on the tenant info. Some of the brokers in the returned list may not in fact serve the table if they are just added to the tenant. This PR fixes the issue by fetching the brokers from the broker external view.

Upgrade Notes

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

Yes (Please label as backward-incompat, and complete the section below on Release Notes)

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

Yes (Please label this as backward-incompat, and complete the section below on Release Notes)

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

codecov-commenter · 2021-10-11T21:26:22Z

Codecov Report

Merging #7556 (f13b213) into master (9ef1f49) will decrease coverage by 2.27%.
The diff coverage is 60.00%.

@@             Coverage Diff              @@
##             master    #7556      +/-   ##
============================================
- Coverage     72.67%   70.39%   -2.28%     
- Complexity     3413     4021     +608     
============================================
  Files          1524     1578      +54     
  Lines         75702    80219    +4517     
  Branches      11039    11908     +869     
============================================
+ Hits          55017    56473    +1456     
- Misses        16995    19878    +2883     
- Partials       3690     3868     +178

Flag	Coverage Δ
integration1	`?`
integration2	`27.73% <0.00%> (-1.41%)`	⬇️
unittests1	`68.61% <ø> (-1.28%)`	⬇️
unittests2	`14.54% <60.00%> (-0.73%)`	⬇️

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	`63.84% <64.28%> (-1.08%)`	⬇️
...pinot/minion/exception/TaskCancelledException.java	`0.00% <0.00%> (-100.00%)`	⬇️
...nverttorawindex/ConvertToRawIndexTaskExecutor.java	`0.00% <0.00%> (-100.00%)`	⬇️
...plugin/segmentuploader/SegmentUploaderDefault.java	`0.00% <0.00%> (-87.10%)`	⬇️
...ore/startree/executor/StarTreeGroupByExecutor.java	`0.00% <0.00%> (-86.67%)`	⬇️
.../transform/function/MapValueTransformFunction.java	`0.00% <0.00%> (-85.30%)`	⬇️
...ot/common/messages/RoutingTableRebuildMessage.java	`0.00% <0.00%> (-81.82%)`	⬇️
...e/pinot/common/minion/MergeRollupTaskMetadata.java	`0.00% <0.00%> (-78.27%)`	⬇️
...ache/pinot/common/lineage/SegmentLineageUtils.java	`22.22% <0.00%> (-77.78%)`	⬇️
... and 336 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 9ef1f49...f13b213. Read the comment docs.

mqliang

minor comments. LGTM otherwise

mqliang · 2021-10-12T06:37:23Z

pinot-clients/pinot-java-client/src/main/java/org/apache/pinot/client/ExternalViewReader.java

+    return getTableToBrokersMapFromExternalView(true, false);
+  }
+
+  private Map<String, List<String>> getTableToBrokersMapFromExternalView(boolean tableWithType, boolean useUrlFormat) {
    Map<String, Set<String>> brokerUrlsMap = new HashMap<>();
    try {
      byte[] brokerResourceNodeData = _zkClient.readData("/EXTERNALVIEW/brokerResource", true);


You can reuse the BROKER_EXTERNAL_VIEW_PATH constant to replace String "/EXTERNALVIEW/brokerResource" here.

yupeng9 · 2021-10-12T15:43:24Z

pinot-clients/pinot-java-client/src/main/java/org/apache/pinot/client/ExternalViewReader.java

  public Map<String, List<String>> getTableToBrokersMap() {
+    return getTableToBrokersMapFromExternalView(false, true);
+


nil: newline

yupeng9 · 2021-10-12T15:45:33Z

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

+  public List<String> getLiveBrokersForTable(String tableNameWithType) {
+    ExternalViewReader externalViewReader = new ExternalViewReader(new ZkClient(_helixZkURL));
+    Map<String, List<String>> tableToBrokersMap = externalViewReader.getTableWithTypeToRawBrokerInstanceIdsMap();
+    return tableToBrokersMap == null ? null : tableToBrokersMap.get(tableNameWithType);


shall we return empty map other than null?

Revised to return empty map.

Jackie-Jiang

Let's not modify the existing API and change its behavior. Users might rely on it for other purpose other than routing the queries.
I'd suggest adding another API to get the live brokers for a given table

pinot-controller/pom.xml

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

Jackie-Jiang · 2021-10-13T17:53:36Z

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

+      @ApiResponse(code = 500, message = "Internal server error")})
+  public String getTableBrokers(
+      @ApiParam(value = "Table name without type", required = true) @PathParam("tableName") String tableName) {
+    ObjectNode ret = JsonUtils.newObjectNode();


Let's not reuse the response format for this new API. Would suggest just returning an array of live brokers.

If the input is a table name with type, returns the live brokers for the table, or 404 if table is not found

If the input is a raw table name, if both offline and realtime table exist, return the brokers live for both tables (we should only route query to the broker with both tables online); if only one table exist, return the live brokers for the single table; if no table is found, return 404

I think we should make this API to return as close to what the zookeeper stores as possible. Today the zk stores one typed table entries. It is better to returns the typed table entries as well. In this way this API can serve multiple purposes than just querying routing. For example, one may even use it to detect if there is any problem with hybrid table's broker assignment. We can leave it to the client to do what they want to do with the returned results -- they can do intersection if they want to.

IMO, 99% of the usage for this API would be to find the brokers to route the query, and returning brokers for each individual table and let the user do the intersection is not easy to use, and can confuse the user. If we do want to get the brokers for the individual table, we can put the table name with type. This way, the API is easy to use, and it can also achieve the debugging purpose.

Refactor to return the list of live brokers given a tablename with type.

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

.../main/java/org/apache/pinot/controller/helix/core/util/BrokerResourceExternalViewReader.java

Jackie-Jiang

LGTM otherwise

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

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

… brokers of a table. (apache#7556) * Let getTableInstance() call to return the list of live brokers. * Fix lint errors. * Fix lint errors. * Fix unit tests * Revise based on comments. * Fix based on comments. * Add a new controller end point. * Fix linters. * Revise based on comments * Revise based on comments * Revised based on comments.

Let getTableInstance() call to return the list of live brokers.

e2e3aeb

chenboat requested a review from Jackie-Jiang October 11, 2021 19:21

chenboat added 3 commits October 11, 2021 13:32

Fix lint errors.

79a96cf

Fix lint errors.

53f4aac

Fix unit tests

6ac7f45

chenboat requested a review from yupeng9 October 11, 2021 21:40

mqliang approved these changes Oct 12, 2021

View reviewed changes

yupeng9 reviewed Oct 12, 2021

View reviewed changes

Revise based on comments.

ed67337

chenboat requested a review from yupeng9 October 12, 2021 18:31

yupeng9 approved these changes Oct 12, 2021

View reviewed changes

Jackie-Jiang reviewed Oct 12, 2021

View reviewed changes

pinot-controller/pom.xml Outdated Show resolved Hide resolved

...-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableInstances.java Outdated Show resolved Hide resolved

chenboat added 3 commits October 12, 2021 17:24

Fix based on comments.

4e6fdaa

Add a new controller end point.

52c9daf

Fix linters.

6831431

Jackie-Jiang reviewed Oct 13, 2021

View reviewed changes

chenboat added 2 commits October 26, 2021 16:34

Revise based on comments

04788f5

Revise based on comments

4e871fb

chenboat requested a review from Jackie-Jiang October 26, 2021 23:38

Jackie-Jiang approved these changes Oct 29, 2021

View reviewed changes

Revised based on comments.

f13b213

chenboat merged commit fbb9e8e into apache:master Oct 29, 2021

chenboat deleted the table_broker_fix branch November 1, 2021 17:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Let the controller getTableInstance() call to return the list of live brokers of a table. #7556

Let the controller getTableInstance() call to return the list of live brokers of a table. #7556

chenboat commented Oct 11, 2021

codecov-commenter commented Oct 11, 2021 •

edited

Loading

mqliang left a comment

mqliang Oct 12, 2021

chenboat Oct 12, 2021

yupeng9 Oct 12, 2021

chenboat Oct 12, 2021

yupeng9 Oct 12, 2021

chenboat Oct 12, 2021 •

edited

Loading

Jackie-Jiang left a comment

Jackie-Jiang Oct 13, 2021

chenboat Oct 14, 2021

Jackie-Jiang Oct 14, 2021

chenboat Oct 26, 2021

Jackie-Jiang left a comment

		public Map<String, List<String>> getTableToBrokersMap() {
		return getTableToBrokersMapFromExternalView(false, true);

Let the controller getTableInstance() call to return the list of live brokers of a table. #7556

Let the controller getTableInstance() call to return the list of live brokers of a table. #7556

Conversation

chenboat commented Oct 11, 2021

Description

Upgrade Notes

Release Notes

Documentation

codecov-commenter commented Oct 11, 2021 • edited Loading

Codecov Report

mqliang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenboat Oct 12, 2021 • edited Loading

Choose a reason for hiding this comment

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jackie-Jiang left a comment

Choose a reason for hiding this comment

codecov-commenter commented Oct 11, 2021 •

edited

Loading

chenboat Oct 12, 2021 •

edited

Loading