xiangfu0 commented on code in PR #20222:
URL: https://github.com/apache/pinot/pull/20222#discussion_r4536614460
##########
pinot-controller/src/main/java/org/apache/pinot/controller/util/TableMetadataReader.java:
##########
@@ -131,92 +131,38 @@ private TableReloadJsonResponse
processSegmentMetadataReloadResponse(
public JsonNode getSegmentsMetadata(String tableNameWithType, @Nullable
List<String> columns,
@Nullable List<String> segments, int timeoutMs)
throws InvalidConfigException, IOException {
- return getSegmentsMetadataInternal(tableNameWithType, columns, segments,
timeoutMs);
- }
+ Map<String, List<String>> serverToSegs =
+ _pinotHelixResourceManager.getServerToSegmentsMap(tableNameWithType);
+ BiMap<String, String> endpoints =
+
_pinotHelixResourceManager.getDataInstanceAdminEndpoints(serverToSegs.keySet());
+ ServerSegmentMetadataReader reader =
+ new ServerSegmentMetadataReader(_executor, _connectionManager,
+ _pinotHelixResourceManager.getServerAdminAuthProvider());
- /// Common helper used by both the new (server-level) and legacy
(segment-level) endpoints.
- private JsonNode fetchAndAggregateMetadata(List<String> urls, BiMap<String,
String> endpoints, boolean perSegmentJson,
- String tableNameWithType, int timeoutMs)
- throws InvalidConfigException, IOException {
- CompletionServiceHelper cs = new CompletionServiceHelper(_executor,
_connectionManager, endpoints,
+ List<String> tableUrls = new ArrayList<>(serverToSegs.size());
+ for (String server : serverToSegs.keySet()) {
+ tableUrls.add(reader.generateTableMetadataServerURL(
+ tableNameWithType, columns, segments, endpoints.get(server)));
+ }
+
+ CompletionServiceHelper cs = new CompletionServiceHelper(_executor,
_connectionManager, endpoints.inverse(),
_pinotHelixResourceManager.getServerAdminAuthProvider());
CompletionServiceHelper.CompletionServiceResponse resp =
- cs.doMultiGetRequest(urls, tableNameWithType, perSegmentJson,
timeoutMs);
- // all requests will fail if new server endpoint is not available
+ cs.doMultiGetRequest(tableUrls, tableNameWithType, false, timeoutMs);
if (resp._failedResponseCount > 0) {
Review Comment:
The fallback did more than support servers older than 1.5.0. It also
recovered from any table-level failure on current servers. I ran a local probe
with 2 servers, where server1's table-level endpoint returned 404, returned
500, or timed out. At the merge-base, all three returned the full metadata. At
this head, all three throw `Got 1 failed responses...`. A server that was
completely down failed both before and after.
This means a per-request timeout (`serverAdminRequestTimeoutSeconds`) on a
large server with `columns=*` used to succeed and now errors. Dropping it seems
reasonable, since the fallback sent one request per segment. Please mention it
in the description/upgrade note so the behavior change is deliberate.
##########
pinot-controller/src/test/java/org/apache/pinot/controller/util/TableMetadataReaderTest.java:
##########
@@ -150,17 +143,10 @@ private HttpServer startServer(List<String> segments)
byte[] tableLevelBody =
JsonUtils.objectToString(tableLevelResponse).getBytes(StandardCharsets.UTF_8);
HttpServer httpServer = HttpServer.create(new
InetSocketAddress("localhost", 0), 0);
- httpServer.createContext("/tables/", exchange -> {
- String path = exchange.getRequestURI().getPath();
- if (path.equals("/tables/" + TABLE_NAME + "/segments/metadata")) {
- exchange.sendResponseHeaders(200, tableLevelBody.length);
- try (OutputStream out = exchange.getResponseBody()) {
- out.write(tableLevelBody);
- }
- } else {
- _legacyRequestCount.incrementAndGet();
- exchange.sendResponseHeaders(404, -1);
- exchange.close();
+ httpServer.createContext("/tables/" + TABLE_NAME + "/segments/metadata",
exchange -> {
Review Comment:
Fail-fast is now the contract, but no test covers it. Consider adding a case
where one server returns non-2xx or times out, and assert that
`getSegmentsMetadata` throws. That guards against a later change silently
returning partial metadata.
--
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]
