Jackie-Jiang commented on code in PR #15637:
URL: https://github.com/apache/pinot/pull/15637#discussion_r2059255083
##########
pinot-controller/src/main/java/org/apache/pinot/controller/util/TableMetadataReader.java:
##########
@@ -71,25 +72,26 @@ public TableReloadJsonResponse
getServerCheckSegmentsReloadMetadata(String table
return processSegmentMetadataReloadResponse(segmentsMetadataResponse);
}
+ /**
+ * Only send needReload request to servers that are part of the
ExternalView. The tagged server list should not be
+ * used as it may be outdated and may not handle scenarios like tiered
storage and COMPLETED segments.
+ * needReload throws an exception for servers that don't contain segments
for the given table
+ */
public ServerSegmentMetadataReader.TableReloadResponse
getReloadCheckResponses(String tableNameWithType,
int timeoutMs) throws InvalidConfigException {
- TableType tableType =
TableNameBuilder.getTableTypeFromTableName(tableNameWithType);
- List<String> serverInstances =
_pinotHelixResourceManager.getServerInstancesForTable(tableNameWithType,
tableType);
- Set<String> serverInstanceSet = new HashSet<>(serverInstances);
+ ExternalView externalView =
_pinotHelixResourceManager.getTableExternalView(tableNameWithType);
+ Set<String> serverInstanceSet =
getCurrentlyAssignedServersFromExternalView(externalView);
return getServerSetReloadCheckResponses(tableNameWithType, timeoutMs,
serverInstanceSet);
}
- /**
- * Check if segments need a reload on any servers based on a provided server
set (useful for rebalance where the
- * currently assigned servers may not match the currently tagged server list)
- * @return response containing a) number of failed responses, b) reload
responses returned
- */
- public TableReloadJsonResponse
getServerSetCheckSegmentsReloadMetadata(String tableNameWithType,
- int timeoutMs, Set<String> serverSet)
- throws InvalidConfigException, IOException {
- ServerSegmentMetadataReader.TableReloadResponse segmentsMetadataResponse =
getServerSetReloadCheckResponses(
- tableNameWithType, timeoutMs, serverSet);
- return processSegmentMetadataReloadResponse(segmentsMetadataResponse);
+ private Set<String> getCurrentlyAssignedServersFromExternalView(ExternalView
externalView) {
+ Map<String, Map<String, String>> assignment = externalView != null ?
externalView.getRecord().getMapFields()
Review Comment:
Move the `null` check to the caller side, and we can short circuit the case
when EV doesn't exist
##########
pinot-controller/src/main/java/org/apache/pinot/controller/util/TableMetadataReader.java:
##########
@@ -71,25 +72,26 @@ public TableReloadJsonResponse
getServerCheckSegmentsReloadMetadata(String table
return processSegmentMetadataReloadResponse(segmentsMetadataResponse);
}
+ /**
+ * Only send needReload request to servers that are part of the
ExternalView. The tagged server list should not be
+ * used as it may be outdated and may not handle scenarios like tiered
storage and COMPLETED segments.
+ * needReload throws an exception for servers that don't contain segments
for the given table
+ */
public ServerSegmentMetadataReader.TableReloadResponse
getReloadCheckResponses(String tableNameWithType,
int timeoutMs) throws InvalidConfigException {
- TableType tableType =
TableNameBuilder.getTableTypeFromTableName(tableNameWithType);
- List<String> serverInstances =
_pinotHelixResourceManager.getServerInstancesForTable(tableNameWithType,
tableType);
- Set<String> serverInstanceSet = new HashSet<>(serverInstances);
+ ExternalView externalView =
_pinotHelixResourceManager.getTableExternalView(tableNameWithType);
+ Set<String> serverInstanceSet =
getCurrentlyAssignedServersFromExternalView(externalView);
return getServerSetReloadCheckResponses(tableNameWithType, timeoutMs,
serverInstanceSet);
}
- /**
- * Check if segments need a reload on any servers based on a provided server
set (useful for rebalance where the
- * currently assigned servers may not match the currently tagged server list)
- * @return response containing a) number of failed responses, b) reload
responses returned
- */
- public TableReloadJsonResponse
getServerSetCheckSegmentsReloadMetadata(String tableNameWithType,
- int timeoutMs, Set<String> serverSet)
- throws InvalidConfigException, IOException {
- ServerSegmentMetadataReader.TableReloadResponse segmentsMetadataResponse =
getServerSetReloadCheckResponses(
- tableNameWithType, timeoutMs, serverSet);
- return processSegmentMetadataReloadResponse(segmentsMetadataResponse);
+ private Set<String> getCurrentlyAssignedServersFromExternalView(ExternalView
externalView) {
+ Map<String, Map<String, String>> assignment = externalView != null ?
externalView.getRecord().getMapFields()
+ : new HashMap<>();
+ Set<String> servers = new HashSet<>();
+ for (Map<String, String> serverStateMap : assignment.values()) {
+ servers.addAll(serverStateMap.keySet());
Review Comment:
Skip `OFFLINE` and `ERROR` segments
--
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: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
