ibessonov commented on code in PR #7428:
URL: https://github.com/apache/ignite-3/pull/7428#discussion_r2703633563
##########
modules/table/src/integrationTest/java/org/apache/ignite/internal/table/ItPartitionTableStatsMetricTest.java:
##########
@@ -276,10 +280,43 @@ void reachMilestoneUpdateTest() {
}
}
+ @Test
+ void pendingWriteIntentsMetric() {
+ String tabName = "test_table_pending_wi";
+
+ sql(format("CREATE TABLE {}(id INT PRIMARY KEY, val INT) ZONE {};",
tabName, ZONE_1_PART_NO_REPLICAS));
+
+ Transaction tx = CLUSTER.aliveNode().transactions().begin();
+
+ int inserts = 3;
+
+ try {
+ for (int i = 0; i < inserts; i++) {
+ sql(tx, format("INSERT INTO {} VALUES(?, ?);", tabName), i, i);
+ }
+
+ expectPendingWriteIntents(tabName, inserts);
+
+ // The writes are still uncommitted, so the modification counter
must not change yet.
+ expectModsCount(tabName, 0);
+ } finally {
+ tx.commit();
+ }
+
+ expectPendingWriteIntents(tabName, 0);
Review Comment:
Please run this test several hundred or thousand times. I'm afraid that it
might be unstable. Write intents cleanup is an asynchronous process, as far as
I know, and here you're just hoping that it is completed between `tx.commit()`
and `expectPendingWriteIntents(tabName, 0)`, which is not guaranteed.
I think you should use awaitility to wait until this method stops throwing
assertions. Not sure about the next one, please check if there are any
transitive guarantees for `modsCount` value. Maybe there aren't any
##########
modules/table/src/integrationTest/java/org/apache/ignite/internal/table/ItPartitionTableStatsMetricTest.java:
##########
@@ -276,10 +280,43 @@ void reachMilestoneUpdateTest() {
}
}
+ @Test
+ void pendingWriteIntentsMetric() {
+ String tabName = "test_table_pending_wi";
Review Comment:
We don't usually use short "abbreviations" like `tab` in Ignite 3. Ignite 2
and Ignite 3 have different code style guidelines, please keep that in mind.
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/StorageUpdateHandler.java:
##########
@@ -506,6 +506,15 @@ public IndexUpdateHandler getIndexUpdateHandler() {
return indexUpdateHandler;
}
+ /**
+ * Returns the total number of unresolved write intents across all
transactions.
+ *
+ * @return Total number of pending row IDs.
+ */
+ public int getPendingRowAmount() {
+ return pendingRows.getPendingRowAmount();
+ }
Review Comment:
I believe it's `Count`, not `Amount`. Rows are countable, you can say "3
rows" and it'll be grammatically correct. I'd also use `Rows`, not `Row`
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -1648,49 +1650,57 @@ private PartitionUpdateHandlers
createPartitionUpdateHandlers(
PartitionModificationCounter modificationCounter =
partitionModificationCounterFactory.create(partSizeSupplier,
table::stalenessConfiguration, table.tableId(), partitionId);
- registerPartitionModificationCounterMetrics(table, partitionId,
modificationCounter);
-
StorageUpdateHandler storageUpdateHandler = new StorageUpdateHandler(
partitionId,
partitionDataStorage,
indexUpdateHandler,
replicationConfiguration,
modificationCounter
);
+
storageUpdateHandler.start(onNodeRecovery);
+ registerPartitionModificationCounterMetrics(table, partitionId,
modificationCounter, storageUpdateHandler::getPendingRowAmount);
Review Comment:
Please rename this method as well, so that it matches the name of metric
source
--
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]
