Copilot commented on code in PR #16691:
URL: https://github.com/apache/pinot/pull/16691#discussion_r2300043444
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/mailbox/MailboxService.java:
##########
@@ -168,4 +169,14 @@ public ReceivingMailbox getReceivingMailbox(String
mailboxId) {
public void releaseReceivingMailbox(ReceivingMailbox mailbox) {
_receivingMailboxCache.invalidate(mailbox.getId());
}
+
+ private static Duration getIdleTimeout(PinotConfiguration config) {
+ long channelIdleTimeoutSeconds = config.getProperty(
+
CommonConstants.MultiStageQueryRunner.KEY_OF_CHANNEL_IDLE_TIMEOUT_SECONDS,
+
CommonConstants.MultiStageQueryRunner.DEFAULT_CHANNEL_IDLE_TIMEOUT_SECONDS);
+ if (channelIdleTimeoutSeconds > 0) {
+ return Duration.ofSeconds(channelIdleTimeoutSeconds);
+ }
+ return Duration.ofSeconds(Integer.MAX_VALUE);
Review Comment:
Using Integer.MAX_VALUE seconds (approximately 68 years) as idle timeout may
cause integer overflow when converted to milliseconds internally by gRPC.
Consider using a more reasonable maximum value like Duration.ofDays(365) or
handle the negative timeout case differently.
```suggestion
// Use a reasonable maximum idle timeout (1 year) to avoid overflow.
return Duration.ofDays(365);
```
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -1903,6 +1903,22 @@ public static class MultiStageQueryRunner {
public static final String KEY_OF_MAX_INBOUND_QUERY_DATA_BLOCK_SIZE_BYTES
= "pinot.query.runner.max.msg.size.bytes";
public static final int DEFAULT_MAX_INBOUND_QUERY_DATA_BLOCK_SIZE_BYTES =
16 * 1024 * 1024;
+
+ /**
+ * Configuration for channel idle timeout in seconds.
+ *
+ * gRPC channels go idle after a period of inactivity. When a channel is
idle, its resources are released. The next
+ * query using the channel will need to re-establish the connection. This
includes the TLS negotiation and therefore
+ * can increase the latency of the query by some milliseconds.
+ *
+ * In normal Pinot clusters where that are continuously serving queries,
channels should never go idle.
Review Comment:
Grammatical error: 'where that are' should be 'that are' - remove the word
'where'.
```suggestion
* In normal Pinot clusters that are continuously serving queries,
channels should never go idle.
```
##########
pinot-common/src/main/java/org/apache/pinot/common/config/GrpcConfig.java:
##########
@@ -23,7 +23,18 @@
import org.apache.pinot.common.utils.tls.TlsUtils;
import org.apache.pinot.spi.env.PinotConfiguration;
-
+/// Configs used for the gRPC **query** service.
+///
+/// Remember that in Pinot we use different gPRC services for different
purposes:
+/// - **query**: used by Pinot users for executing queries
+/// - **internal**: used by Pinot for internal communication between
servers/broker in MSE. This includes:
+/// - The ability to send query plans from broker to server, and the
mailbox service for sending data between
+/// servers/brokers
+/// - The broker -> server communication for the SSE when
`pinot.broker.request.handler.type` is set to grpc
+/// (see the GrpcBrokerRequestHandler)?
Review Comment:
The question mark at the end makes this documentation unclear. Either
confirm this information and remove the question mark, or rephrase to indicate
uncertainty more clearly.
```suggestion
/// (see the GrpcBrokerRequestHandler)
```
--
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]
