You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@phoenix.apache.org by "lokiore (via GitHub)" <gi...@apache.org> on 2026/08/25 00:10:03 UTC
[PR] PHOENIX-7995 Tag client HA metrics with the HA group and add missing HA/CRR/failover client metrics [phoenix]
lokiore opened a new pull request, #2609:
URL: https://github.com/apache/phoenix/pull/2609
### What changes were proposed in this pull request?
This PR makes the ZK-less HA client's HA/CRR/failover metrics sliceable per HA group, and fills gaps in the client-side HA metric coverage. It builds on (and supersedes) #2605, which is folded in here as its own PHOENIX-7872 commit.
**1. Per-HA-group metrics tag.** A new per-group Hadoop Metrics2 source, `HAGroupClientMetricsSource`, registers one metrics2 source per HA group. Each instance:
- appends `,haGroup=<ObjectName.quote(name)>` to its JMX context so every group registers as a distinct source / MBean, and
- stamps a `haGroup` tag carrying the (unquoted) group name so the series can be sliced per HA group downstream.
`HAGroupMetricsManager` is a small registry (one source per group name) wired into `HighAvailabilityGroup` lifecycle: a source is created on group init and detached on group close. Emission is **dual**: the existing JVM-global `GLOBAL_HA_*` counters continue to emit unchanged, and the same event is additionally recorded on the per-group source. The tag key lives in a new module-neutral `org.apache.phoenix.metrics.MetricConstants` (`HA_GROUP_TAG_NAME = "haGroup"`), referenced by both the new client source and the server-side `HAGroupStoreMetricsSource` so both sides share one tag key.
The per-group set is intentionally limited to metrics attributable to a single HA group (each emission site has a `HighAvailabilityGroup` in scope). The JVM-shared parallel-executor pool metrics and the `HA_CRR_CACHE_AGE_MS` gauge are excluded.
**2. Missing client counters.** New `MetricType`s and their `GLOBAL_HA_*` wrappers, emitted (dual) at the relevant client sites:
- `HA_FAILOVER_CONNECTION_CREATED_COUNTER` — a `FailoverPhoenixConnection` was successfully created against the active cluster.
- `HA_FAILOVER_CONNECTION_FAILED_COUNTER` — a connect-to-active attempt threw (no active cluster, demoted mid-connect, or connect error). *(from #2605 / PHOENIX-7872)*
- `HA_ROLE_TRANSITION_FAILED_COUNTER` — a cluster-role-transition dispatch failed while applying a new CRR.
- `CRR_TRANSITION_COUNT` — a CRR transition was applied per HA policy, including transitions into a no-active state (distinct from `HA_FAILOVER_COUNT`, which counts only transitions that establish/move an ACTIVE cluster).
Existing counters (`HA_FAILOVER_COUNT`, `HA_FAILOVER_DURATION_MS`, `HA_STALE_CRR_DETECTED_COUNT`, `HA_MUTATION_BLOCKED_COUNT`, `HA_CRR_REFRESH_COUNT`, the `HA_PARALLEL_*` connection/task counters, and the poller-tick counters) are additionally recorded per-group at their existing emission sites.
The #2605 change (record `HA_FAILOVER_DURATION_MS` on the CRR-write path rather than the dead `failover()` path, and add the connection-failed counter) is preserved as its own PHOENIX-7872-attributed commit at the base of this branch; #2605 will be closed as superseded by this PR.
### Why are the changes needed?
On the ZK-less HA client the JVM-global `GLOBAL_HA_*` counters aggregate across every HA group in the process, so a JVM serving more than one HA group cannot attribute failover / stale-CRR / mutation-blocked / transition activity to a specific group. Tagging each series with `haGroup` makes per-group dashboards and alerting possible while keeping the existing global counters intact for backward compatibility. The additional counters close observability gaps around connection creation/failure and role-transition outcomes that had no client-side metric.
### Does this PR introduce _any_ user-facing change?
No behavioral change. New client-side metrics are added (new `MetricType` entries and their `GLOBAL_HA_*` wrappers) and a new per-group `haGroup`-tagged metrics2 source is registered; the existing global HA counters are unchanged. No SQL, API, or wire-format change.
### How was this patch tested?
New unit tests (run under surefire):
- `HAGroupClientMetricsSourceTest` — the `haGroup` tag carries the unquoted group name; the JMX context is quoted per group; increment/update are per-counter; unknown metric types are ignored; each group is a distinct registered source; unregister frees the source name for reuse.
- `HAGroupMetricsManagerTest` — `getOrCreate` is idempotent and registers a source; null/empty group names are a no-op; two groups never cross-count; each group gets a distinct tagged source; `update` accumulates per group; `remove` detaches the source; re-create after remove rebuilds a fresh source.
- `HighAvailabilityGroupTest` additions cover the CRR-write-path duration/connection-failed emission (from #2605 / PHOENIX-7872).
`mvn spotless:apply` was run before pushing; the full module build compiles clean.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 4.8)
--
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: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7995 Tag client HA metrics with the HA group and add missing HA/CRR/failover client metrics [phoenix]
Posted by "lokiore (via GitHub)" <gi...@apache.org>.
lokiore merged PR #2609:
URL: https://github.com/apache/phoenix/pull/2609
--
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: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7995 Tag client HA metrics with the HA group and add missing HA/CRR/failover client metrics [phoenix]
Posted by "himanshu-gwalani (via GitHub)" <gi...@apache.org>.
himanshu-gwalani commented on code in PR #2609:
URL: https://github.com/apache/phoenix/pull/2609#discussion_r3871552203
##########
phoenix-core-client/src/main/java/org/apache/phoenix/jdbc/HighAvailabilityGroup.java:
##########
@@ -1199,6 +1218,11 @@ public boolean refreshClusterRoleRecord(boolean forceRefresh) throws SQLExceptio
final ClusterRoleRecord oldRecord = roleRecord;
state = State.IN_TRANSITION;
+ // Count every applied CRR transition, including transitions into a no-active state. Distinct
+ // from HA_FAILOVER_COUNT, which counts only transitions that establish/move an ACTIVE
+ // cluster.
+ GLOBAL_HA_CRR_TRANSITION_COUNT.increment();
Review Comment:
This is to count for number of requests of transition or the actual number of transitions applied?
Currently it increments before the transition is successful, so in case transition is not applied, counter would still be incremented.
##########
phoenix-core-client/src/main/java/org/apache/phoenix/monitoring/HAGroupClientMetricsSource.java:
##########
@@ -0,0 +1,131 @@
+/*
+ * 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.phoenix.monitoring;
+
+import static org.apache.phoenix.monitoring.MetricType.CRR_TRANSITION_COUNT;
+import static org.apache.phoenix.monitoring.MetricType.HA_CRR_REFRESH_COUNT;
+import static org.apache.phoenix.monitoring.MetricType.HA_FAILOVER_CONNECTION_CREATED_COUNTER;
+import static org.apache.phoenix.monitoring.MetricType.HA_FAILOVER_CONNECTION_FAILED_COUNTER;
+import static org.apache.phoenix.monitoring.MetricType.HA_FAILOVER_COUNT;
+import static org.apache.phoenix.monitoring.MetricType.HA_FAILOVER_DURATION_MS;
+import static org.apache.phoenix.monitoring.MetricType.HA_MUTATION_BLOCKED_COUNT;
+import static org.apache.phoenix.monitoring.MetricType.HA_PARALLEL_CONNECTION_CREATED_COUNTER;
+import static org.apache.phoenix.monitoring.MetricType.HA_PARALLEL_CONNECTION_ERROR_COUNTER;
+import static org.apache.phoenix.monitoring.MetricType.HA_PARALLEL_CONNECTION_FALLBACK_COUNTER;
+import static org.apache.phoenix.monitoring.MetricType.HA_PARALLEL_TASK_TIMEOUT_COUNTER;
+import static org.apache.phoenix.monitoring.MetricType.HA_POLLER_TICK_COUNT;
+import static org.apache.phoenix.monitoring.MetricType.HA_POLLER_TICK_FAILURES;
+import static org.apache.phoenix.monitoring.MetricType.HA_ROLE_TRANSITION_FAILED_COUNTER;
+import static org.apache.phoenix.monitoring.MetricType.HA_STALE_CRR_DETECTED_COUNT;
+
+import java.util.EnumMap;
+import java.util.Map;
+import javax.management.ObjectName;
+import org.apache.hadoop.hbase.metrics.BaseSourceImpl;
+import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem;
+import org.apache.hadoop.metrics2.lib.Interns;
+import org.apache.hadoop.metrics2.lib.MutableFastCounter;
+import org.apache.phoenix.metrics.MetricConstants;
+
+import org.apache.phoenix.thirdparty.com.google.common.annotations.VisibleForTesting;
+
+/**
+ * Per-HA-group Hadoop Metrics2 source for the ZK-less HA client's HA/CRR/failover metrics.
+ * <p>
+ * All groups share one metrics name/context; each instance appends {@code ,haGroup=<quoted-name>}
+ * to the JMX context so it registers as a distinct metrics2 source / MBean, and stamps a
+ * {@code ha_group} tag ({@link MetricConstants#HA_GROUP_TAG_NAME}) carrying the (unquoted) group
+ * name so the series can be sliced per HA group downstream. This mirrors the server-side
+ * {@code HAGroupStoreMetricsSourceImpl} tagging pattern; both reference the same tag key.
+ * <p>
+ * Each metric in {@link #METRIC_TYPES} is a monotonic {@link MutableFastCounter}. Accumulating
+ * metrics such as {@code HA_FAILOVER_DURATION_MS} add total milliseconds via {@link #update}, which
+ * matches the semantics of the JVM-global {@code GLOBAL_HA_*} counters; those continue to emit
+ * unchanged alongside this per-group source (dual emit).
+ * <p>
+ * The set is intentionally limited to metrics attributable to a single HA group (each emission site
+ * has a {@code HighAvailabilityGroup} in scope). The shared parallel-executor pool metrics
+ * ({@code HA_PARALLEL_POOL1_*}/{@code HA_PARALLEL_POOL2_*}) and the {@code HA_CRR_CACHE_AGE_MS}
+ * gauge are excluded: the pools are JVM-shared and the gauge is not a counter.
+ */
+public class HAGroupClientMetricsSource extends BaseSourceImpl {
+
+ static final String METRICS_NAME = "HAGroupClient";
+ static final String METRICS_DESC = "Phoenix HA Group Client Metrics";
+ static final String METRICS_CONTEXT = "phoenix";
+ static final String METRICS_JMX_CONTEXT = "Phoenix,sub=" + METRICS_NAME;
+
+ /**
+ * The HA metrics attributable to a single HA group; each emission site has a
+ * {@code HighAvailabilityGroup} in scope.
+ */
+ public static final MetricType[] METRIC_TYPES = new MetricType[] { HA_FAILOVER_COUNT,
Review Comment:
nit: Not used outside this class, so can be private (and also unmodifiable - to avoid accidental updates)
--
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: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7995 Tag client HA metrics with the HA group and add missing HA/CRR/failover client metrics [phoenix]
Posted by "tkhurana (via GitHub)" <gi...@apache.org>.
tkhurana commented on PR #2609:
URL: https://github.com/apache/phoenix/pull/2609#issuecomment-5429913602
1. Unguarded metrics registration on the HA-group init path — HighAvailabilityGroup.java:648. getOrCreate(getName()) is the one emission entry point not wrapped, and it
constructs a HAGroupClientMetricsSource, whose BaseSourceImpl ctor registers with DefaultMetricsSystem and can throw MetricsException on a name collision. init()
declares throws SQLException — so an observability failure would propagate into HA-group initialization, the exact inversion of the PR's stated "metrics never break the
request path." The increment/update paths are correctly guarded; this one isn't. Fix: make getOrCreate swallow-and-log construction/registration failures (the shared
choke point) or wrap the call site. This same unguarded path also means a late poller tick after close()/remove() could silently re-register an orphaned source — the
getOrCreate guard covers both.
2. Misleading "pairs with" on CREATED vs FAILED counters (comment-only) — FailoverPhoenixConnection.java:115-116 and MetricType.java:369.
HA_FAILOVER_CONNECTION_CREATED_COUNTER increments once, only in the FailoverPhoenixConnection ctor; HA_FAILOVER_CONNECTION_FAILED_COUNTER increments inside
connectActive, which is reached from more sites — the failover() retry loop (once per failed attempt) and the HighAvailabilityPolicy fallback. They are not a matched
pair, so failed/(created+failed) is not a valid failure rate. Reword both comments to say FAILED counts every failed active-connect attempt across all callers (incl.
per-retry) while CREATED counts only constructed FailoverPhoenixConnection instances. (Flagged by both code-reviewer and comment-analyzer.)
3. Per-group emission is untested at the call sites — HighAvailabilityGroupTest.java. The whole PR thesis is "dual emit," but the new tests assert only GLOBAL_HA_*; the
per-group half has zero call-site coverage (a dropped increment, wrong MetricType, or wrong group name would pass every test). Nearly free to close: add
HAGroupMetricsManager.getIfPresent(name).getCounterValue(...) assertions inside the two existing tests. Also untested: the getOrCreate-on-READY / remove-on-close
lifecycle (tests use the State.READY ctor that bypasses init() and never call close()), and emission of HA_ROLE_TRANSITION_FAILED_COUNTER / CRR_TRANSITION_COUNT (the
failing-transition path is never driven).
--
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: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org