Jackie-Jiang commented on a change in pull request #8188:
URL: https://github.com/apache/pinot/pull/8188#discussion_r812405289
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -3181,15 +3181,54 @@ public TableStats getTableStats(String
tableNameWithType) {
}
/**
- * Return the list of live brokers serving the corresponding table.
- * Each entry in the broker list is of the following format:
- * Broker_hostname_port
+ * 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 tableNameWithType) {
+ public List<String> getLiveBrokersForTable(String tableName)
+ throws TableNotFoundException {
ExternalView ev =
_helixDataAccessor.getProperty(_keyBuilder.externalView(Helix.BROKER_RESOURCE_INSTANCE));
if (ev == null) {
- return Collections.EMPTY_LIST;
+ return new ArrayList<>();
}
+ TableType inputTableType =
TableNameBuilder.getTableTypeFromTableName(tableName);
+ if (inputTableType != null) {
+ if (!hasTable(tableName)) {
+ throw new TableNotFoundException(String.format("Table=%s not found",
tableName));
+ }
+ return getLiveBrokersForTable(ev, tableName);
+ }
+ String offlineTableName =
TableNameBuilder.OFFLINE.tableNameWithType(tableName);
+ String realtimeTableName =
TableNameBuilder.REALTIME.tableNameWithType(tableName);
+ boolean hasOfflineTable = hasTable(offlineTableName);
+ boolean hasRealtimeTable = hasTable(realtimeTableName);
+ if (!hasOfflineTable && !hasRealtimeTable) {
+ throw new TableNotFoundException(String.format("Table=%s not found",
tableName));
+ }
+ if (hasOfflineTable && hasRealtimeTable) {
+ Set<String> offlineTables = new HashSet<>(getLiveBrokersForTable(ev,
offlineTableName));
+ return getLiveBrokersForTable(ev, realtimeTableName).stream()
Review comment:
(code format) Can you setup the IDE with the latest [Pinot
Style](https://docs.pinot.apache.org/developers/developers-and-contributors/code-setup#intellij)
and reformat the changes
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -3181,15 +3181,54 @@ public TableStats getTableStats(String
tableNameWithType) {
}
/**
- * Return the list of live brokers serving the corresponding table.
- * Each entry in the broker list is of the following format:
- * Broker_hostname_port
+ * 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 tableNameWithType) {
+ public List<String> getLiveBrokersForTable(String tableName)
+ throws TableNotFoundException {
ExternalView ev =
_helixDataAccessor.getProperty(_keyBuilder.externalView(Helix.BROKER_RESOURCE_INSTANCE));
if (ev == null) {
- return Collections.EMPTY_LIST;
+ return new ArrayList<>();
Review comment:
We can `throw new IllegalStateException("Failed to find external view
for " + Helix.BROKER_RESOURCE_INSTANCE)`, and the rest API should return 500
##########
File path:
pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManagerTest.java
##########
@@ -939,8 +944,73 @@ 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));
+ }
+
+ // Create the realtime table.
+ ControllerTestUtils.addDummySchema(REALTIME_TABLE_NAME);
+ tableConfig = new TableConfigBuilder(TableType.REALTIME)
+ .setTableName(TABLE_NAME)
Review comment:
(code format) Reformat the changes
--
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]
