xiangfu0 commented on code in PR #19148:
URL: https://github.com/apache/pinot/pull/19148#discussion_r3700483873
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/materializedview/MaterializedViewTaskExecutorFactory.java:
##########
@@ -65,12 +66,12 @@ public PinotTaskExecutor create() {
// Build the gRPC client config from the minion's own configuration,
scoped to the
// MaterializedViewTask.MINION_BROKER_GRPC_CONFIG_PREFIX prefix.
This is how operators
// enable TLS, raise the max inbound message size for large MV
result sets, and tune
- // keepalive. Falling back to an empty configuration (no TLS,
defaults) when no
- // MinionConf was provided — fine for local tests but production
deployments should
- // initialize the factory with a MinionConf.
- PinotConfiguration grpcClientConfig = _minionConf != null
- ?
_minionConf.subset(MaterializedViewTask.MINION_BROKER_GRPC_CONFIG_PREFIX)
- : new PinotConfiguration();
+ // keepalive. Defaulting silently would build a plaintext client
with default limits,
+ // so fail loudly instead.
+ Preconditions.checkState(_minionConf != null,
+ "MinionConf is not set; init(zkMetadataManager, minionConf) must
be called before create()");
+ PinotConfiguration grpcClientConfig =
+
_minionConf.subset(MaterializedViewTask.MINION_BROKER_GRPC_CONFIG_PREFIX);
Review Comment:
Good catch — added a `_zkMetadataManager` check too, and moved both to the
top of `create()`: inside the lazy-init block they were skipped entirely once
`_queryExecutor` had been built, so they now run on every call. Fixed in
fa287ec.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableInstances.java:
##########
@@ -172,7 +172,9 @@ public Map<String, List<InstanceInfo>>
getLiveBrokers(@Context HttpHeaders heade
try {
return
_pinotHelixResourceManager.getTableToLiveBrokersMapping(headers.getHeaderString(DATABASE),
tables);
} catch (Exception e) {
- throw new ControllerApplicationException(LOGGER, e.getMessage(),
Response.Status.NOT_FOUND);
+ // Unknown tables are filtered out rather than reported, so anything
thrown here (e.g. a missing broker
+ // ExternalView) is a server-side failure, not a lookup miss.
+ throw new ControllerApplicationException(LOGGER, e.getMessage(),
Response.Status.INTERNAL_SERVER_ERROR, e);
Review Comment:
Agreed — switched to the stable message "Failed to get table to live brokers
mapping" and left the cause attached for the log detail; a null `getMessage()`
would otherwise have emptied both the log line and the 500 response body. Fixed
in fa287ec.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
--
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]
