xiangfu0 commented on code in PR #19892: URL: https://github.com/apache/pinot/pull/19892#discussion_r4288261907
########## pinot-broker/src/main/java/org/apache/pinot/broker/routing/FailOpenPolicy.java: ########## @@ -0,0 +1,69 @@ +/** + * 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. + * + * <p>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). + */ +@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; + } + + public static FailOpenPolicy disabled() { + return new FailOpenPolicy(false, false); + } + + public boolean isEnabled() { Review Comment: **CRITICAL** — `isEnabled()` is never read anywhere on the routing or query path. I grepped the whole `pinot-broker/src/main` tree and the only references to `_failOpenPolicy` are: the field, the constructor call, and the listener registration in `BaseBrokerStarter`. Nothing in `BaseInstanceSelector.select()` or the request handlers consults it, so the broker never actually routes to an unhealthy server when a replica is missing. As written, the policy is dead state and the feature described in the PR title isn't implemented. Please wire `isEnabled()` into the selection/routing logic (and cover it with a test that shows routing actually changes when the flag flips), or clarify that this PR only lands the metrics + config plumbing. ########## pinot-broker/src/main/java/org/apache/pinot/broker/routing/FailOpenPolicy.java: ########## @@ -0,0 +1,69 @@ +/** + * 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. + * + * <p>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). + */ +@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; + } + + public static FailOpenPolicy disabled() { Review Comment: **MINOR** — the `disabled()` static factory has no callers in the production code. If it's only meant for tests, consider marking it `@VisibleForTesting`; otherwise it can be dropped to keep the surface lean. ########## pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/BaseInstanceSelector.java: ########## @@ -446,6 +447,9 @@ void refreshSegmentStates() { + "(all candidate instances: {} are disabled, counting segment as unavailable)", segment, Review Comment: **MINOR** — when `candidates` is empty this still logs "all candidate instances: [] are disabled", which reads oddly since there are no candidates at all. Since you now track that distinction via `segmentsWithoutCandidates`, consider a clearer message for the no-candidates case. ########## pinot-common/src/main/java/org/apache/pinot/common/metrics/BrokerMeter.java: ########## @@ -216,6 +216,14 @@ public class BrokerMeter implements AbstractMetrics.Meter { public static final BrokerMeter ROUTING_TABLE_UPDATE_FAILURE = create("ROUTING_TABLE_UPDATE_FAILURE", "failures", false); + // Panic routing metrics Review Comment: **MINOR** — the comment says "Panic routing metrics" but everything else uses the "fail-open" terminology. Worth aligning the wording so the metric's intent is clear to whoever reads it later. ########## pinot-common/src/main/java/org/apache/pinot/common/metrics/BrokerMeter.java: ########## @@ -216,6 +216,14 @@ public class BrokerMeter implements AbstractMetrics.Meter { public static final BrokerMeter ROUTING_TABLE_UPDATE_FAILURE = create("ROUTING_TABLE_UPDATE_FAILURE", "failures", false); + // Panic routing metrics Review Comment: **MINOR** — the comment says "Panic routing metrics" but everything else uses the "fail-open" terminology. Worth aligning the wording so the metric's intent is clear to whoever reads it later. ########## pinot-broker/src/main/java/org/apache/pinot/broker/routing/FailOpenPolicy.java: ########## @@ -0,0 +1,69 @@ +/** + * 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. + * + * <p>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). + */ +@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; + } + + public static FailOpenPolicy disabled() { Review Comment: **MINOR** — the `disabled()` static factory has no callers in the production code. If it's only meant for tests, consider marking it `@VisibleForTesting`; otherwise it can be dropped to keep the surface lean. ########## pinot-broker/src/main/java/org/apache/pinot/broker/routing/FailOpenPolicy.java: ########## @@ -0,0 +1,69 @@ +/** + * 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. + * + * <p>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). + */ +@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; + } + + public static FailOpenPolicy disabled() { + return new FailOpenPolicy(false, false); + } + + public boolean isEnabled() { Review Comment: **CRITICAL** — `isEnabled()` is never read anywhere on the routing or query path. I grepped the whole `pinot-broker/src/main` tree and the only references to `_failOpenPolicy` are: the field, the constructor call, and the listener registration in `BaseBrokerStarter`. Nothing in `BaseInstanceSelector.select()` or the request handlers consults it, so the broker never actually routes to an unhealthy server when a replica is missing. As written, the policy is dead state and the feature described in the PR title isn't implemented. Please wire `isEnabled()` into the selection/routing logic (and cover it with a test that shows routing actually changes when the flag flips), or clarify that this PR only lands the metrics + config plumbing. ########## pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/BaseInstanceSelector.java: ########## @@ -446,6 +447,9 @@ void refreshSegmentStates() { + "(all candidate instances: {} are disabled, counting segment as unavailable)", segment, Review Comment: **MINOR** — when `candidates` is empty this still logs "all candidate instances: [] are disabled", which reads oddly since there are no candidates at all. Since you now track that distinction via `segmentsWithoutCandidates`, consider a clearer message for the no-candidates case. -- 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]
