gortiz commented on code in PR #16560:
URL: https://github.com/apache/pinot/pull/16560#discussion_r2299708831
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/mailbox/MailboxService.java:
##########
@@ -159,4 +175,26 @@ public ReceivingMailbox getReceivingMailbox(String
mailboxId) {
public void releaseReceivingMailbox(ReceivingMailbox mailbox) {
_receivingMailboxCache.invalidate(mailbox.getId());
}
+
+ private static Duration getPingerPeriod(PinotConfiguration config) {
+ long pingerPeriodSecs = config.getProperty(
+ CommonConstants.MultiStageQueryRunner.KEY_OF_PINGER_PERIOD_SECONDS,
+ CommonConstants.MultiStageQueryRunner.DEFAULT_PINGER_PERIOD_SECONDS);
+ return Duration.ofSeconds(pingerPeriodSecs);
+ }
+
+ 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);
+ }
+ Duration pingerPeriod = getPingerPeriod(config);
+ if (pingerPeriod.isNegative() || pingerPeriod.isZero()) {
+ return Duration.ofMinutes(30);
+ } else {
+ return pingerPeriod.multipliedBy(2);
+ }
Review Comment:
Ideally, we would like to:
* If the DEFAULT_CHANNEL_IDLE_TIMEOUT_SECONDS is not negative, use that
number of seconds. This is useful mostly to manually configure gRPC using an
unsafe config (that can end up having inactive channels) to test that scenario.
* If the DEFAULT_CHANNEL_IDLE_TIMEOUT_SECONDS is negative (the default):
* If the pinger is enabled (the default): Set the idle timeout to twice as
long as the pinger period, so we are sure we don't end up timing out.
* If the pinger is disabled, keep using the default value
30 mins is the default value, so this is what I set here. Alternatively, I
could return a null Duration and only decorate the channel if that duration is
not null. Still, I decided to set the default here to avoid using nulls
explicitly. Do you think he other way around is better?
--
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]
