bitflicker64 commented on code in PR #3357: URL: https://github.com/apache/hugegraph/pull/3357#discussion_r4287795304
########## hugegraph-pd/hg-pd-service/src/main/java/org/apache/hugegraph/pd/metrics/StoreStateMetrics.java: ########## @@ -0,0 +1,157 @@ +/* + * 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.hugegraph.pd.metrics; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.HashMap; +import java.util.Iterator; +import java.util.List; +import java.util.Map; + +import org.apache.hugegraph.pd.common.PDException; +import org.apache.hugegraph.pd.grpc.Metapb; + +import io.micrometer.core.instrument.Gauge; +import io.micrometer.core.instrument.Meter; +import io.micrometer.core.instrument.MeterRegistry; +import lombok.extern.slf4j.Slf4j; + +/** + * Per-store gauges built from the store and shard group metadata PD already keeps: + * {@code hg.store.state{store,state}} is 1 for the store's current state and 0 for the + * others, and {@code hg.store.leader.count{store}} counts the shard groups it leads. + * Stores are registered and removed at runtime, so the meters follow the store list on + * every refresh. The gauges, and {@code hg.stores} for stores not yet registered, trigger + * the refresh themselves at most once per {@link #REFRESH_INTERVAL_MS}, so one scrape reads + * the metadata once. + */ +@Slf4j +final class StoreStateMetrics { + + static final long REFRESH_INTERVAL_MS = 1000; + + @FunctionalInterface + interface Source<T> { + + T get() throws PDException; + } + + private final MeterRegistry registry; + private final Source<List<Metapb.Store>> stores; + private final Source<List<Metapb.ShardGroup>> shardGroups; + // Guarded by this + private final Map<Long, List<Meter>> meters = new HashMap<>(); + private long lastRefresh; + + private volatile Map<Long, Metapb.StoreState> states = Collections.emptyMap(); + private volatile Map<Long, Integer> leaders = Collections.emptyMap(); + + StoreStateMetrics(MeterRegistry registry, Source<List<Metapb.Store>> stores, + Source<List<Metapb.ShardGroup>> shardGroups) { + this.registry = registry; + this.stores = stores; + this.shardGroups = shardGroups; + } + + /** + * Reads the store list and shard groups, registers meters for new stores and removes + * those of stores PD no longer knows. On a read error the previous values are kept. + */ + synchronized void refresh() { + List<Metapb.Store> storeList; + List<Metapb.ShardGroup> groups; + try { + storeList = this.stores.get(); + groups = this.shardGroups.get(); + } catch (PDException | RuntimeException e) { + // RuntimeException: a scrape can arrive before PDService finishes init + log.warn("Failed to read stores for store state metrics", e); + return; Review Comment: Minor: On a failed read, `refresh()` returns before it sets `lastRefresh`, so `refreshIfStale()` stays true for the rest of the scrape and every gauge retries the read. Each of the six `hg_store_state` gauges and the `hg_store_leader_count` gauge of every store calls `refreshIfStale()`, and `hg_stores` calls it once more. With N stores, one scrape then makes 7N+1 RocksDB scans and logs 7N+1 WARN lines, each with a full stack trace. That is the case the class Javadoc rules out ("at most once per REFRESH_INTERVAL_MS, so one scrape reads the metadata once"). I checked it at this head with a scratch test: 3 stores, a `PrometheusMeterRegistry` scrape, and a source that throws `PDException`. It counted 22 reads in one scrape, against 1 when reads succeed. A persistent KV read error therefore floods the PD log on every scrape, at the moment an operator most needs that log. Please set `this.lastRefresh` before the read, or in a `finally`, so a failure is retried at most once per interval. The previous values are still kept. Please also add an assertion to `testReadErrorKeepsLastValues` that reading all of a store's gauges after a failure calls the source once. -- 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]
