Copilot commented on code in PR #7984:
URL: https://github.com/apache/incubator-seata/pull/7984#discussion_r2945610351
##########
rm-datasource/pom.xml:
##########
@@ -168,5 +168,10 @@
<artifactId>json-common</artifactId>
<version>${project.version}</version>
</dependency>
+ <dependency>
+ <groupId>${project.groupId}</groupId>
+ <artifactId>seata-metrics-core</artifactId>
+ <version>${project.version}</version>
+ </dependency>
Review Comment:
Only `seata-metrics-core` is added, but the default metrics registry type is
`compact` (see `DefaultValues.DEFAULT_METRICS_REGISTRY_TYPE`), whose
implementation lives in `seata-metrics-registry-compact`. Without also
including a registry implementation (or explicitly disabling metrics by default
for RM), `RegistryFactory.getInstance()` will fail to load a provider and
metrics will never record (and may cause repeated warning logs). Add the
appropriate registry dependency (or switch to `seata-metrics-all`) and/or
ensure metrics are disabled unless the registry/exporter is present.
##########
rm-datasource/src/main/java/org/apache/seata/rm/datasource/undo/AbstractUndoLogManager.java:
##########
@@ -190,6 +205,16 @@ public void batchDeleteUndoLog(Set<String> xids, Set<Long>
branchIds, Connection
e = new SQLException(e);
}
throw (SQLException) e;
+ } finally {
+ Registry registry = getRegistry();
+ if (registry != null) {
+
registry.getTimer(UndoLogConstants.TIMER_UNDO_LOG_DELETE_LATENCY)
+ .record(System.nanoTime() - start,
TimeUnit.NANOSECONDS);
+ if (totalDeleteRows > 0) {
+
registry.getCounter(UndoLogConstants.COUNTER_UNDO_LOG_DELETE_COUNT)
+ .increase(totalDeleteRows);
Review Comment:
In `batchDeleteUndoLog`, the delete counter is increased by
`totalDeleteRows` (rows affected by the main delete), while `deleteUndoLog`
always increases by 1. This makes the `COUNTER_UNDO_LOG_DELETE_COUNT` metric
semantics inconsistent (operation count vs row count) and also ignores rows
deleted by `deleteSubPST`. Align the counter to a single meaning (e.g.,
increment by 1 per delete call, or track deleted-row counts separately and
include both statements).
##########
rm-datasource/src/main/java/org/apache/seata/rm/datasource/undo/AbstractUndoLogManager.java:
##########
@@ -586,4 +615,19 @@ public boolean hasUndoLogTable(Connection conn) {
protected String getCheckUndoLogTableExistSql() {
return CHECK_UNDO_LOG_TABLE_EXIST_SQL;
}
+
+ /**
+ * Get the metrics registry instance.
+ * This method is protected to allow testing with Mockito spy.
+ *
+ * @return the Registry instance, or null if metrics are not enabled
+ */
+ protected Registry getRegistry() {
+ try {
+ return RegistryFactory.getInstance();
+ } catch (Throwable t) {
+ LOGGER.warn("Failed to get metrics registry: {}", t.getMessage());
+ return null;
+ }
Review Comment:
`getRegistry()` calls `RegistryFactory.getInstance()` on every undo-log
flush/delete, which repeatedly hits config + service-loading and can become a
hot-path overhead. Consider caching the resolved `Registry` (including a cached
"unavailable" state) and avoid logging on every call; also log the exception
with stack trace (current code logs only `t.getMessage()`, losing diagnosis).
##########
rm-datasource/src/test/java/org/apache/seata/rm/datasource/undo/UndoLogMetricsTest.java:
##########
@@ -0,0 +1,100 @@
+/*
+ * 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.seata.rm.datasource.undo;
+
+import org.apache.seata.metrics.Id;
+import org.apache.seata.metrics.registry.Registry;
+import org.apache.seata.rm.datasource.ConnectionContext;
+import org.apache.seata.rm.datasource.ConnectionProxy;
+import org.apache.seata.rm.datasource.DataSourceProxy;
+import org.apache.seata.rm.datasource.undo.mysql.MySQLUndoLogManager;
+import org.junit.jupiter.api.Test;
+
+import java.sql.Connection;
+import java.sql.PreparedStatement;
+import java.util.Collections;
+import java.util.concurrent.TimeUnit;
+
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+public class UndoLogMetricsTest {
+
+ @Test
+ public void testFlushUndoLogsMetrics() throws Exception {
+ Registry registry = mock(Registry.class);
+ org.apache.seata.metrics.Summary summary =
mock(org.apache.seata.metrics.Summary.class);
+ when(registry.getSummary(any(Id.class))).thenReturn(summary);
+
+ ConnectionProxy connectionProxy = mock(ConnectionProxy.class);
+ ConnectionContext context = mock(ConnectionContext.class);
+ DataSourceProxy dataSourceProxy = mock(DataSourceProxy.class);
+
+ when(connectionProxy.getContext()).thenReturn(context);
+ when(connectionProxy.getDataSourceProxy()).thenReturn(dataSourceProxy);
+ Connection connection = mock(Connection.class);
+ PreparedStatement preparedStatement = mock(PreparedStatement.class);
+
when(connection.prepareStatement(any(String.class))).thenReturn(preparedStatement);
+ when(connectionProxy.getTargetConnection()).thenReturn(connection);
+
when(dataSourceProxy.getResourceId()).thenReturn("jdbc:mysql://localhost:3306/test");
+
+ when(context.hasUndoLog()).thenReturn(true);
+ when(context.getXid()).thenReturn("xid");
+ when(context.getBranchId()).thenReturn(123456L);
+ when(context.getUndoItems()).thenReturn(Collections.emptyList());
+
+ // Spy on the manager to mock getRegistry()
+ MySQLUndoLogManager manager = spy(new MySQLUndoLogManager());
+ doReturn(registry).when(manager).getRegistry();
+
+ manager.flushUndoLogs(connectionProxy);
+
+
verify(registry).getSummary(eq(UndoLogConstants.SUMMARY_UNDO_LOG_SIZE));
+ verify(summary).increase(anyLong());
+ }
+
+ @Test
+ public void testDeleteUndoLogMetrics() throws Exception {
+ Registry registry = mock(Registry.class);
+ org.apache.seata.metrics.Timer timer =
mock(org.apache.seata.metrics.Timer.class);
+ org.apache.seata.metrics.Counter counter =
mock(org.apache.seata.metrics.Counter.class);
+
+ when(registry.getTimer(any(Id.class))).thenReturn(timer);
+ when(registry.getCounter(any(Id.class))).thenReturn(counter);
+
+ Connection connection = mock(Connection.class);
+ PreparedStatement preparedStatement = mock(PreparedStatement.class);
+
when(connection.prepareStatement(any(String.class))).thenReturn(preparedStatement);
+
+ // Spy on the manager to mock getRegistry()
+ MySQLUndoLogManager manager = spy(new MySQLUndoLogManager());
+ doReturn(registry).when(manager).getRegistry();
+
+ manager.deleteUndoLog("xid", 123L, connection);
+
+
verify(registry).getTimer(eq(UndoLogConstants.TIMER_UNDO_LOG_DELETE_LATENCY));
+ verify(timer).record(anyLong(), any(TimeUnit.class));
+
verify(registry).getCounter(eq(UndoLogConstants.COUNTER_UNDO_LOG_DELETE_COUNT));
+ verify(counter).increase(1);
+ }
Review Comment:
The production code instruments both `deleteUndoLog` and
`batchDeleteUndoLog`, but this test class only verifies metrics for
`flushUndoLogs` and the single-row delete path. Add a test that exercises
`batchDeleteUndoLog` and asserts the expected counter semantics (especially if
the counter is meant to represent operations vs deleted rows).
--
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]
