FrankChen021 commented on code in PR #19459:
URL: https://github.com/apache/druid/pull/19459#discussion_r3234370473
##########
sql/src/main/java/org/apache/druid/sql/calcite/schema/SystemServerPropertiesTable.java:
##########
@@ -108,42 +121,62 @@ public Schema.TableType getJdbcTableType()
}
@Override
- public Enumerable<Object[]> scan(DataContext root)
+ public Enumerable<Object[]> scan(
+ final DataContext root,
+ final List<RexNode> filters,
Review Comment:
[P2] Actually apply pushed filters before fetching every server
The new ProjectableFilterableTable scan receives Calcite filters, but this
method never inspects or removes any accepted filters. Calcite will apply the
remaining filters after the scan, so queries such as `WHERE server =
'host:port'` still enumerate all discovered servers and call
`/status/properties` for each one. That means an unrelated slow or unreachable
node can still delay a query that should have been scoped to one server, and
the advertised filter pushdown is not actually implemented. Please parse and
consume supported filters, at least `server` and likely
`service_name`/`property`, before fetching properties.
##########
sql/src/main/java/org/apache/druid/sql/calcite/schema/SystemServerPropertiesTable.java:
##########
@@ -154,20 +187,34 @@ private Map<String, String> getProperties(DruidNode
druidNode)
.get();
if (response.getStatus().getCode() != HttpServletResponse.SC_OK) {
- throw new RE(
- "Failed to get properties from node[%s]. Error code[%d],
description[%s].",
- url,
- response.getStatus().getCode(),
- response.getStatus().getReasonPhrase()
- );
+ final String errorMsg = StringUtils.format("HTTP %d: %s",
+
response.getStatus().getCode(),
+
response.getStatus().getReasonPhrase());
+ log.warn("Failed to get properties from node[%s]: error[%s]", url,
errorMsg);
+ return new PropertiesResult(new HashMap<>(), errorMsg);
}
- return jsonMapper.readValue(
- response.getContent(),
- new TypeReference<>(){}
+ return new PropertiesResult(
+ jsonMapper.readValue(response.getContent(), new TypeReference<>(){}),
+ null
);
}
catch (Exception e) {
Review Comment:
[P2] Do not turn interrupts into error rows
`Future.get()` can throw `InterruptedException`, but the broad catch now
converts it into a synthetic `error_message` row and clears the interrupt
status. If the SQL query is cancelled or the execution thread is interrupted
during a properties fetch, the scan can keep iterating and return partial table
data instead of aborting. Handle `InterruptedException` separately by
re-interrupting the thread and propagating cancellation rather than treating it
as an unreachable Druid server.
--
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]
