Jackie-Jiang commented on a change in pull request #8188:
URL: https://github.com/apache/pinot/pull/8188#discussion_r810276532
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableInstances.java
##########
@@ -51,8 +53,7 @@
@Produces(MediaType.APPLICATION_JSON)
@ApiOperation(value = "List table instances", notes = "List instances of the
given table")
@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"),
Review comment:
revert this
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -3180,12 +3180,56 @@ public TableStats getTableStats(String
tableNameWithType) {
return new TableStats(creationTime);
}
+ /**
+ * Return the list of live brokers serving the corresponding table. Based on
the
+ * input tableName, there can be 3 cases:
+ *
+ * 1. If the tableName has a type-suffix, then brokers for only that
table-type
+ * will be returned.
+ * 2. If the tableName doesn't have a type-suffix and there's only 1 type
for that
+ * table, then the brokers for that table-type would be returned.
+ * 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
+ * broker which can serve both table-types.
+ *
+ * @param tableName name of table with or without type suffix.
+ * @return list of brokers serving the given table in the format:
Broker_hostname_port.
+ * @throws TableNotFoundException when no table exists with the given name.
+ */
+ public List<String> getLiveBrokersForTable(String tableName)
+ throws TableNotFoundException {
+ TableType inputTableType =
TableNameBuilder.getTableTypeFromTableName(tableName);
+ if (inputTableType != null) {
+ if (!hasTable(tableName)) {
+ throw new TableNotFoundException(String.format("Table=%s not found",
tableName));
+ }
+ return getLiveBrokersForTable(tableName, inputTableType);
+ }
+ boolean hasOfflineTable = hasOfflineTable(tableName);
Review comment:
(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.
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -3180,12 +3180,56 @@ public TableStats getTableStats(String
tableNameWithType) {
return new TableStats(creationTime);
}
+ /**
+ * Return the list of live brokers serving the corresponding table. Based on
the
Review comment:
+1, very good documentation
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableInstances.java
##########
@@ -116,16 +117,25 @@ public String getTableInstances(
}
@GET
- @Path("/tables/{tableNameWithType}/livebrokers")
+ @Path("/tables/{tableName}/livebrokers")
@Produces(MediaType.APPLICATION_JSON)
@ApiOperation(value = "List the brokers serving a table", notes = "List live
brokers of the given table based on EV")
@ApiResponses(value = {
@ApiResponse(code = 200, message = "Success"),
@ApiResponse(code = 404, message = "Table not found"),
- @ApiResponse(code = 500, message = "Internal server error")})
+ @ApiResponse(code = 500, message = "Internal server error")
+ })
public List<String> getLiveBrokersForTable(
- @ApiParam(value = "Table name with type", required = true)
- @PathParam("tableNameWithType") String tableNameWithType) {
- return
_pinotHelixResourceManager.getLiveBrokersForTable(tableNameWithType);
+ @ApiParam(value = "Table name (with or without type)", required = true)
+ @PathParam("tableName") String tableName) {
+ try {
+ return _pinotHelixResourceManager.getLiveBrokersForTable(tableName);
+ } catch (TableNotFoundException tableNotFoundException) {
+ throw new WebApplicationException(String.format("Table=%s not found",
tableName), 404);
Review comment:
We have some example of returning table not found in
`PinotTableRestletResource`
```suggestion
throw new ControllerApplicationException(LOGGER, String.format("Table
'%s' does not exist", tableName),
Response.Status.NOT_FOUND);
```
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableInstances.java
##########
@@ -116,16 +117,25 @@ public String getTableInstances(
}
@GET
- @Path("/tables/{tableNameWithType}/livebrokers")
+ @Path("/tables/{tableName}/livebrokers")
@Produces(MediaType.APPLICATION_JSON)
@ApiOperation(value = "List the brokers serving a table", notes = "List live
brokers of the given table based on EV")
@ApiResponses(value = {
@ApiResponse(code = 200, message = "Success"),
@ApiResponse(code = 404, message = "Table not found"),
- @ApiResponse(code = 500, message = "Internal server error")})
+ @ApiResponse(code = 500, message = "Internal server error")
+ })
public List<String> getLiveBrokersForTable(
- @ApiParam(value = "Table name with type", required = true)
- @PathParam("tableNameWithType") String tableNameWithType) {
- return
_pinotHelixResourceManager.getLiveBrokersForTable(tableNameWithType);
+ @ApiParam(value = "Table name (with or without type)", required = true)
+ @PathParam("tableName") String tableName) {
+ try {
+ return _pinotHelixResourceManager.getLiveBrokersForTable(tableName);
+ } catch (TableNotFoundException tableNotFoundException) {
+ throw new WebApplicationException(String.format("Table=%s not found",
tableName), 404);
+ }
+ }
+
+ public void setPinotHelixResourceManager(PinotHelixResourceManager
pinotHelixResourceManager) {
Review comment:
Suggest removing this function and `PinotTableInstancesTest`. The test
in `PinotHelixResourceManagerTest` is good enough, no need to mock a
`PinotHelixResourceManager` for one line of code
##########
File path:
pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManagerTest.java
##########
@@ -939,6 +940,20 @@ public void testGetLiveBrokersForTable()
for (String broker: liveBrokersForTable) {
Assert.assertTrue(broker.startsWith("Broker_localhost"));
}
+
+ // Test retrieving the live broker for table without table-type suffix.
+ liveBrokersForTable =
+
ControllerTestUtils.getHelixResourceManager().getLiveBrokersForTable(TABLE_NAME);
+ Assert.assertEquals(liveBrokersForTable.size(), 2);
+
+ // Test retrieving the live broker for table with non-existent table-type.
+ try {
+
ControllerTestUtils.getHelixResourceManager().getLiveBrokersForTable(REALTIME_TABLE_NAME);
+ Assert.fail("Method call above should have failed");
+ } catch (TableNotFoundException tableNotFoundException) {
+
Assert.assertTrue(tableNotFoundException.getMessage().contains(REALTIME_TABLE_NAME));
+ }
+
Review comment:
Shall we add a test for cases where both OFFLINE and REALTIME table
exist, or none of them exist?
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -3180,12 +3180,56 @@ public TableStats getTableStats(String
tableNameWithType) {
return new TableStats(creationTime);
}
+ /**
+ * Return the list of live brokers serving the corresponding table. Based on
the
+ * input tableName, there can be 3 cases:
+ *
+ * 1. If the tableName has a type-suffix, then brokers for only that
table-type
+ * will be returned.
+ * 2. If the tableName doesn't have a type-suffix and there's only 1 type
for that
+ * table, then the brokers for that table-type would be returned.
+ * 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
+ * broker which can serve both table-types.
+ *
+ * @param tableName name of table with or without type suffix.
+ * @return list of brokers serving the given table in the format:
Broker_hostname_port.
+ * @throws TableNotFoundException when no table exists with the given name.
+ */
+ public List<String> getLiveBrokersForTable(String tableName)
+ throws TableNotFoundException {
+ TableType inputTableType =
TableNameBuilder.getTableTypeFromTableName(tableName);
+ if (inputTableType != null) {
+ if (!hasTable(tableName)) {
+ throw new TableNotFoundException(String.format("Table=%s not found",
tableName));
+ }
+ return getLiveBrokersForTable(tableName, inputTableType);
+ }
+ boolean hasOfflineTable = hasOfflineTable(tableName);
+ boolean hasRealtimeTable = hasRealtimeTable(tableName);
+ if (!hasOfflineTable && !hasRealtimeTable) {
+ throw new TableNotFoundException(String.format("Table=%s not found",
tableName));
+ }
+ if (hasOfflineTable && hasRealtimeTable) {
+ Set<String> offlineTables = new
HashSet<>(getLiveBrokersForTable(tableName, TableType.OFFLINE));
+ return getLiveBrokersForTable(tableName, TableType.REALTIME).stream()
+ .filter(offlineTables::contains)
+ .collect(Collectors.toList());
+ } else {
+ return getLiveBrokersForTable(tableName, hasOfflineTable ?
TableType.OFFLINE : TableType.REALTIME);
+ }
+ }
+
/**
* Return the list of live brokers serving the corresponding table.
* Each entry in the broker list is of the following format:
* Broker_hostname_port
*/
- public List<String> getLiveBrokersForTable(String tableNameWithType) {
+ public List<String> getLiveBrokersForTable(String tableName, TableType
tableType) {
Review comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
