gortiz commented on code in PR #16560:
URL: https://github.com/apache/pinot/pull/16560#discussion_r2299700721
##########
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");
_tlsConfig = tlsConfig;
+ _idleTimeout = idleTimeout;
}
public ManagedChannel getChannel(String hostname, int port) {
// TODO: Revisit parameters
if (_tlsConfig != null) {
return _channelMap.computeIfAbsent(Pair.of(hostname, port),
- (k) -> NettyChannelBuilder
- .forAddress(k.getLeft(), k.getRight())
- .maxInboundMessageSize(
-
CommonConstants.MultiStageQueryRunner.DEFAULT_MAX_INBOUND_QUERY_DATA_BLOCK_SIZE_BYTES)
- .sslContext(ServerGrpcQueryClient.buildSslContext(_tlsConfig))
- .build()
+ (k) -> {
+ NettyChannelBuilder channelBuilder = NettyChannelBuilder
+ .forAddress(k.getLeft(), k.getRight())
+ .maxInboundMessageSize(
+
CommonConstants.MultiStageQueryRunner.DEFAULT_MAX_INBOUND_QUERY_DATA_BLOCK_SIZE_BYTES)
+ .sslContext(ServerGrpcQueryClient.buildSslContext(_tlsConfig));
+ return decorate(channelBuilder).build();
+ }
);
} else {
return _channelMap.computeIfAbsent(Pair.of(hostname, port),
- (k) -> ManagedChannelBuilder
- .forAddress(k.getLeft(), k.getRight())
- .maxInboundMessageSize(
-
CommonConstants.MultiStageQueryRunner.DEFAULT_MAX_INBOUND_QUERY_DATA_BLOCK_SIZE_BYTES)
- .usePlaintext()
- .build());
+ (k) -> {
+ ManagedChannelBuilder<?> channelBuilder = ManagedChannelBuilder
+ .forAddress(k.getLeft(), k.getRight())
+ .maxInboundMessageSize(
+
CommonConstants.MultiStageQueryRunner.DEFAULT_MAX_INBOUND_QUERY_DATA_BLOCK_SIZE_BYTES)
+ .usePlaintext();
+ return decorate(channelBuilder).build();
+ });
}
}
+
+ private ManagedChannelBuilder<?> decorate(ManagedChannelBuilder<?> builder) {
+ return builder.idleTimeout(_idleTimeout.getSeconds(), TimeUnit.SECONDS);
+ }
Review Comment:
It is a place to write all the modifications we need to add to the
ManagedChannelBuilder, independnetly on whether tls is enabled or not. Probably
we reformat the code to make it a bit better (ie have instead a
createChannelBuilder(tlsConfig) method hat returns the customized
ManagedChannelBuilder and then just add all the common stuff on getChannel),
but this is the solution I found that requires less repetition and at the same
time less modification on the current 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]
