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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
