Copilot commented on code in PR #2492:
URL: https://github.com/apache/phoenix/pull/2492#discussion_r3352233309
##########
phoenix-core-server/src/main/java/org/apache/phoenix/replication/metrics/MetricsReplicationLogGroupSource.java:
##########
@@ -33,17 +33,33 @@ public interface MetricsReplicationLogGroupSource extends
BaseSource {
String ROTATION_FAILURES = "rotationFailures";
String ROTATION_FAILURES_DESC = "Number of times log rotation has failed";
- String APPEND_TIME = "appendTimeMs";
+
+ // All time histograms in this source are nanoseconds.
+ String APPEND_TIME = "appendTime";
String APPEND_TIME_DESC = "Histogram of time taken for append operations in
nanoseconds";
- String SYNC_TIME = "syncTimeMs";
+ String SYNC_TIME = "syncTime";
Review Comment:
This change renames existing JMX histogram keys (e.g., `appendTimeMs` ->
`appendTime`, etc.). That will break any external dashboards/alerts scraping
the prior names. If we need to preserve compatibility, keep the legacy metric
keys (even if the units are nanoseconds) or register aliases for the old names.
##########
phoenix-core/src/it/java/org/apache/phoenix/replication/ReplicationLogGroupIT.java:
##########
@@ -178,6 +179,22 @@ private void verifyReplication(Map<String, Integer>
expected) throws Exception {
}
}
+ private void assertMetricsEmitted() {
+ ReplicationLogMetricValues values =
logGroup.getMetrics().getCurrentMetricValues();
+ assertTrue("appendTime should be > 0, got " + values.getAppendTime(),
+ values.getAppendTime() > 0);
+ assertTrue("syncTime should be > 0, got " + values.getSyncTime(),
values.getSyncTime() > 0);
+ assertTrue("ringBufferTime should be > 0, got " +
values.getRingBufferTime(),
+ values.getRingBufferTime() > 0);
+ assertTrue("fsSyncTime should be > 0, got " + values.getFsSyncTime(),
+ values.getFsSyncTime() > 0);
+ assertTrue("batchSize should be > 0, got " + values.getBatchSize(),
values.getBatchSize() > 0);
+ assertTrue("pendingSyncCount should be > 0, got " +
values.getPendingSyncCount(),
+ values.getPendingSyncCount() > 0);
+ assertTrue("pendingSyncWaitTime should be > 0, got " +
values.getPendingSyncWaitTime(),
+ values.getPendingSyncWaitTime() > 0);
Review Comment:
`pendingSyncWaitTime` can legitimately be 0ns (e.g., if the SYNC event is
the last event in the batch and `processPendingSyncs()` runs immediately).
Asserting `> 0` here makes the IT test potentially flaky even when the metric
is emitted.
--
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]
