lokiore commented on code in PR #2627:
URL: https://github.com/apache/phoenix/pull/2627#discussion_r4138536216
##########
phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java:
##########
@@ -859,55 +897,56 @@ public void
preBatchMutate(ObserverContext<RegionCoprocessorEnvironment> c,
// gates below key off this name, not off writer presence, so they apply
on a standby or
// demoted cluster too — not only where this cluster is active.
Optional<String> haGroupName = getHAGroupNameFromBatch(miniBatchOp);
-
- // Path-coverage counter: increments whenever a mutation batch reaches
preBatchMutate
- // without a resolvable HA group attribute, so the cluster-role-based
mutation-block gate
- // has no haGroupName to evaluate against and is skipped. This counts
the code path being
- // short-circuited — it does NOT imply any safety property was breached
(when the block
- // feature is disabled or no block window is active, there is no
property to breach).
- // Tracked globally rather than per-table so operators can compare
baseline vs.
- // post-deploy delta to spot new write paths that forgot to attach
_HAGroupName.
- // Intentionally scoped to !haGroupName.isPresent() regardless of
dataTableName —
- // system-HA-group writes WITH a haGroup are an intended gate exemption
(state writes
- // must proceed during a block window) and are not counted here.
- if (shouldReplicate && !haGroupName.isPresent()) {
+ BatchOrigin origin = classifyBatch(miniBatchOp, haGroupName);
+
+ // Path-coverage counter: increments only for a genuine client write
that reached the write
+ // path without attaching _HAGroupName (CLIENT_NON_HA), on a
replication-eligible table. The
+ // signal is "a client missed attaching _HAGroupName" — it does NOT
imply any safety property
+ // was breached. Tracked globally rather than per-table so operators can
compare baseline vs.
+ // post-deploy delta to spot new write paths that forgot the attribute.
Standby replay
+ // (PHX_REPLAY) and native-replicated-in (NATIVE_IN) batches are
distinct origins and so are
+ // excluded, as are system-HA-group state writes (which carry a haGroup,
i.e. CLIENT_HA).
+ if (replicateTable && origin == BatchOrigin.CLIENT_NON_HA) {
try {
MetricsHaBypassSourceFactory.getInstance().incrementBypassedMutationBlockCount();
} catch (Throwable t) {
LOG.warn("Failed to increment bypassed mutation block count metric;
continuing", t);
}
}
- // We don't want to check for mutation blocking for the system ha group
table
- if (!dataTableName.equals(SYSTEM_HA_GROUP_NAME) &&
haGroupName.isPresent()) {
+ // Cluster-role mutation-block / staleness gate. All HA semantics are
gated on the master
+ // switch (syncReplicationEnabled): with the feature off this never runs
— including the
+ // rollout window where clients already send _HAGroupName but the HA
group does not exist yet.
+ // The system ha group table is exempt so its state writes proceed
during a block window.
+ if (
+ syncReplicationEnabled && origin == BatchOrigin.CLIENT_HA
Review Comment:
Can you put
https://github.com/apache/phoenix/blob/888fa0f457c2f540f7aa5492b0d6acabc477f4c9/phoenix-core-server/src/main/java/org/apache/phoenix/coprocessor/BaseScannerRegionObserver.java#L192
behind the same flag as well so that reads also won't try to read HAGroup as
well
##########
phoenix-core/src/it/java/org/apache/phoenix/replication/ReplicationLogGroupIT.java:
##########
@@ -305,6 +307,85 @@ public void testUncoveredIndexNoCurrentRowState() throws
Exception {
}
}
+ /**
+ * Negative counter case for PHOENIX-XXXX: standby replay batches (origin
{@code PHX_REPLAY})
Review Comment:
Can we remove placeholder here. Thanks
--
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]
