kpatel006 commented on code in PR #19892: URL: https://github.com/apache/pinot/pull/19892#discussion_r4420285039
########## pinot-broker/src/main/java/org/apache/pinot/broker/routing/FailOpenPolicy.java: ########## @@ -0,0 +1,75 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.pinot.broker.routing; + +import java.util.Map; +import java.util.Set; +import javax.annotation.concurrent.ThreadSafe; +import org.apache.pinot.spi.config.provider.PinotClusterConfigChangeListener; +import org.apache.pinot.spi.utils.CommonConstants.Broker; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + + +/// Thread-safe policy for broker fail-open routing. +/// +/// The broker-configured value is retained separately from the effective startup value so removing a dynamic +/// cluster override restores the broker value (or the default when the broker did not configure one). +/// +/// This class currently only lands the config plumbing: the policy is constructed and registered for cluster config +/// changes, but is not yet consulted on the routing path. Instance selection will read [#isEnabled()] in a +/// follow-up change. +@ThreadSafe +public final class FailOpenPolicy implements PinotClusterConfigChangeListener { + private static final Logger LOGGER = LoggerFactory.getLogger(FailOpenPolicy.class); + + private final boolean _brokerConfiguredEnabled; + private volatile boolean _enabled; + + public FailOpenPolicy(boolean brokerConfiguredEnabled, boolean startupEnabled) { + _brokerConfiguredEnabled = brokerConfiguredEnabled; + _enabled = startupEnabled; + } + + /// Returns a policy with fail-open routing disabled and no broker-configured override. Not yet called; intended for + /// routing components that are constructed without a broker-provided policy once fail-open routing is wired in. + public static FailOpenPolicy disabled() { + return new FailOpenPolicy(false, false); + } + + /// Returns whether fail-open routing is currently enabled. Not yet read on the routing path; instance selection will + /// consult this in a follow-up change. + public boolean isEnabled() { Review Comment: > When the selectors start reading this, which excluded replica gets the request, so one sick server doesn't take all the fail-open traffic for its segments? The idea was to round robin the requests. The implementation is in #19908 (round robin [code](https://github.com/apache/pinot/pull/19908/changes/BASE..85662b1c52a030de2596a1d06c3c8285beaa74a5#diff-76f25d0e35652e038f21b32fa406de4124b110cce900b59e3e9adc672750cc66R50-R56)) > And should those requests use a shorter server timeout, since the excluded server is often the one that's timing out? I wanted to treat the request like it would under normal routing, so I didn't use a shorter timeout. Shorter timeout may help, but there may be some genuinely longer lasting queries that may actually take longer. What are your thoughts? -- 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]
