gortiz commented on code in PR #16560:
URL: https://github.com/apache/pinot/pull/16560#discussion_r2299694892
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/mailbox/channel/ChannelManager.java:
##########
@@ -38,30 +43,62 @@
public class ChannelManager {
private final ConcurrentHashMap<Pair<String, Integer>, ManagedChannel>
_channelMap = new ConcurrentHashMap<>();
private final TlsConfig _tlsConfig;
+ /**
+ * The idle timeout for the channel, which cannot be disabled in gRPC.
+ *
+ * In general we want to prevent the channel from going idle, so that we
don't have to re-establish the connection
+ * (including TLS negotiation) before sending any message, which increases
the latency of the first query sent after a
+ * period of inactivity.
+ *
+ * This is why by default we set the idle timeout to twice the pinger period
if a pinger is configured, so that the
+ * pinger can keep the channel alive. In case the pinger is not configured,
we set the idle timeout to 30 minutes,
+ * which is the default value in the gRPC Java implementation.
+ */
+ private final Duration _idleTimeout;
- public ChannelManager(@Nullable TlsConfig tlsConfig) {
+ public ChannelManager(@Nullable TlsConfig tlsConfig, Duration idleTimeout) {
+ Preconditions.checkArgument(idleTimeout.isNegative() ||
idleTimeout.isZero(), "Idle timeout must be positive");
Review Comment:
Good catch! It was a last minute change and I didn't try it. I'll do it
before merging it
--
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]
