Copilot commented on code in PR #19148:
URL: https://github.com/apache/pinot/pull/19148#discussion_r3700443809
##########
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:
The new guard fails loudly when `_minionConf` is missing, but `create()`
also depends on `_zkMetadataManager` (passed into
`MaterializedViewTaskExecutor`) and it can still be null if the factory is used
before `init(...)` (or if `init` is called with a null manager). Consider
validating both required fields so this code always fails fast with a clear
error instead of propagating a null into the executor.
##########
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:
Using `e.getMessage()` as the exception message can be null (many exceptions
have no message), which can lead to an empty/"null" log line and an unhelpful
error response. For a 500, it’s usually better to return a stable, contextual
message and rely on the attached cause for details in logs.
--
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]
