You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "xtern (via GitHub)" <gi...@apache.org> on 2025/11/04 16:44:49 UTC
[PR] IGNITE-26626 Added debug information output to the test (testGlobalMinimumTxRequiredTime) [ignite-3]
xtern opened a new pull request, #6897:
URL: https://github.com/apache/ignite-3/pull/6897
(no comment)
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-26626 Added debug information output to the test (testGlobalMinimumTxRequiredTime) [ignite-3]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #6897:
URL: https://github.com/apache/ignite-3/pull/6897#discussion_r2491322425
##########
modules/catalog-compaction/src/integrationTest/java/org/apache/ignite/internal/catalog/compaction/ItCatalogCompactionTest.java:
##########
@@ -305,4 +333,109 @@ private static void expectEarliestCatalogVersion(int expectedVersion) {
}
});
}
+
+ class DebugInfoCollector {
+ private final List<IgniteImpl> nodes;
+ private final Map<UUID, String> nodesById;
+ private final IgniteStringBuilder buffer = new IgniteStringBuilder("Test debug info").nl();
+ private final List<InternalTransaction> transactions = new ArrayList<>();
+
+ DebugInfoCollector(List<IgniteImpl> nodes) {
+ this.nodes = nodes;
+ Map<UUID, String> nodesById = new HashMap<>();
+
+ for (IgniteImpl node : nodes) {
+ nodesById.put(node.id(), node.name());
+ }
+
+ this.nodesById = nodesById;
+ }
+
+ void recordCatalogState(String contextMessage) {
+ buffer.nl();
+
+ for (IgniteImpl node : nodes) {
+ buffer.app("Catalog state(").app(contextMessage)
+ .app(") on node ").app(node.name()).nl();
+
+ CatalogManager mgr = node.catalogManager();
+
+ for (int ver = mgr.earliestCatalogVersion(); ver < mgr.latestCatalogVersion(); ver++) {
+ Catalog catalog = mgr.catalog(ver);
+
+ buffer.app(" ").app(ver).app(" | ").app(catalog == null ? -1 : catalog.time()).nl();
+ }
+ }
+ }
+
+ void recordMinTxTimesState(String contextMessage) {
+ buffer.nl();
+
+ buffer.app("Minimum RW tx times (").app(contextMessage).app(')').nl();
+
+ for (IgniteImpl node : nodes) {
+ ActiveLocalTxMinimumRequiredTimeProvider timeProvider = node.catalogCompactionRunner()
+ .activeLocalTxMinimumRequiredTimeProvider();
+
+ buffer.app(" ").app(node.name()).app(": ").app(timeProvider.minimumRequiredTime()).nl();
+ }
+ }
+
+ void recordTransactionsState() {
+ // Sort by start time.
+ transactions.sort(Comparator.comparing(t -> beginTimestamp(t.id())));
+
+ List<InternalTransaction> roTransactions = new ArrayList<>();
+ List<InternalTransaction> rwTransactions = new ArrayList<>();
+
+ for (InternalTransaction tx : transactions) {
+ if (tx.isReadOnly()) {
+ roTransactions.add(tx);
+ } else {
+ rwTransactions.add(tx);
+ }
+ }
+
+ buffer.nl();
+ buffer.app("RW transactions state").nl();
+
+ for (InternalTransaction tx : rwTransactions) {
+ buffer.app(" ")
+ .app(tx.isFinishingOrFinished() ? "finished" : "active ").app(" | ")
+ .app(nodesById.get(tx.coordinatorId())).app(" | ")
+ .app(beginTimestamp(tx.id()))
+ .nl();
+ }
+
+ buffer.nl();
+ buffer.app("RO transactions state").nl();
+
+ for (InternalTransaction tx : rwTransactions) {
Review Comment:
Incorrect variable used in loop. This should iterate over `roTransactions` instead of `rwTransactions` to display read-only transaction states. Currently, the RW transactions are being printed twice and RO transactions are never displayed.
```suggestion
for (InternalTransaction tx : roTransactions) {
```
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-26626 Added debug information output to the test (testGlobalMinimumTxRequiredTime) [ignite-3]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #6897:
URL: https://github.com/apache/ignite-3/pull/6897#discussion_r2491435833
##########
modules/catalog-compaction/src/integrationTest/java/org/apache/ignite/internal/catalog/compaction/ItCatalogCompactionTest.java:
##########
@@ -305,4 +333,109 @@ private static void expectEarliestCatalogVersion(int expectedVersion) {
}
});
}
+
+ class DebugInfoCollector {
+ private final List<IgniteImpl> nodes;
+ private final Map<UUID, String> nodesById;
+ private final IgniteStringBuilder buffer = new IgniteStringBuilder("Test debug info").nl();
+ private final List<InternalTransaction> transactions = new ArrayList<>();
+
+ DebugInfoCollector(List<IgniteImpl> nodes) {
+ this.nodes = nodes;
+ Map<UUID, String> nodesById = new HashMap<>();
+
+ for (IgniteImpl node : nodes) {
+ nodesById.put(node.id(), node.name());
+ }
+
+ this.nodesById = nodesById;
+ }
+
+ void recordCatalogState(String contextMessage) {
+ buffer.nl();
+
+ for (IgniteImpl node : nodes) {
+ buffer.app("Catalog state(").app(contextMessage)
+ .app(") on node ").app(node.name()).nl();
+
+ CatalogManager mgr = node.catalogManager();
+
+ for (int ver = mgr.earliestCatalogVersion(); ver < mgr.latestCatalogVersion(); ver++) {
Review Comment:
The loop condition uses `<` instead of `<=`, which excludes the latest catalog version from being recorded in debug output. This should be `ver <= mgr.latestCatalogVersion()` to include all catalog versions in the debug information.
```suggestion
for (int ver = mgr.earliestCatalogVersion(); ver <= mgr.latestCatalogVersion(); ver++) {
```
##########
modules/catalog-compaction/src/integrationTest/java/org/apache/ignite/internal/catalog/compaction/ItCatalogCompactionTest.java:
##########
@@ -305,4 +333,109 @@ private static void expectEarliestCatalogVersion(int expectedVersion) {
}
});
}
+
+ class DebugInfoCollector {
Review Comment:
The inner class `DebugInfoCollector` should be declared as `static` since it doesn't need access to instance members of the outer class `ItCatalogCompactionTest`. This would make the design clearer and prevent unintended access to outer class state.
```suggestion
static class DebugInfoCollector {
```
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-26626 Added debug information output to the test (testGlobalMinimumTxRequiredTime) [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern closed pull request #6897: IGNITE-26626 Added debug information output to the test (testGlobalMinimumTxRequiredTime)
URL: https://github.com/apache/ignite-3/pull/6897
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-26626 Added debug information output to the test (testGlobalMinimumTxRequiredTime) [ignite-3]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #6897:
URL: https://github.com/apache/ignite-3/pull/6897#discussion_r2491435871
##########
modules/catalog-compaction/src/integrationTest/java/org/apache/ignite/internal/catalog/compaction/ItCatalogCompactionTest.java:
##########
@@ -305,4 +333,109 @@ private static void expectEarliestCatalogVersion(int expectedVersion) {
}
});
}
+
+ class DebugInfoCollector {
Review Comment:
The inner class `DebugInfoCollector` should be declared as `static` since it doesn't need access to instance members of the outer class `ItCatalogCompactionTest`. This would make the design clearer and prevent unintended access to outer class state.
```suggestion
static class DebugInfoCollector {
```
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-26626 Added debug information output to the test (testGlobalMinimumTxRequiredTime) [ignite-3]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #6897:
URL: https://github.com/apache/ignite-3/pull/6897#discussion_r2493453821
##########
modules/catalog-compaction/src/integrationTest/java/org/apache/ignite/internal/catalog/compaction/ItCatalogCompactionTest.java:
##########
@@ -138,63 +145,83 @@ void testGlobalMinimumTxRequiredTime() {
IgniteImpl node1 = unwrapIgniteImpl(CLUSTER.node(1));
IgniteImpl node2 = unwrapIgniteImpl(CLUSTER.node(2));
+ DebugInfoCollector debug = new DebugInfoCollector(List.of(node0, node1, node2));
+
List<CatalogCompactionRunner> compactors = List.of(
node0.catalogCompactionRunner(),
node1.catalogCompactionRunner(),
node2.catalogCompactionRunner()
);
- Catalog catalog1 = getLatestCatalog(node2);
+ debug.recordGlobalTxState("init");
+ debug.recordCatalogState("init");
+ debug.recordMinTxTimesState("init");
- Transaction tx1 = beginTx(node0, false);
+ Catalog catalog1 = getLatestCatalog(node2);
+ InternalTransaction tx1 = beginTx(node0, false);
+ debug.recordTx(tx1);
// Changing the catalog and starting transaction.
sql("create table a(a int primary key)");
Catalog catalog2 = getLatestCatalog(node0);
assertThat(catalog2.version(), is(catalog1.version() + 1));
- List<Transaction> txs2 = Stream.of(node1, node2).map(node -> beginTx(node, false)).collect(Collectors.toList());
+ List<InternalTransaction> txs2 = Stream.of(node1, node2).map(node -> beginTx(node, false)).collect(Collectors.toList());
List<InternalTransaction> ignoredReadonlyTxs = Stream.of(node0, node1, node2)
.map(node -> beginTx(node, true))
.collect(Collectors.toList());
+ debug.recordTx(txs2);
+ debug.recordTx(ignoredReadonlyTxs);
+
// Changing the catalog again and starting transaction.
sql("alter table a add column (b int)");
- Awaitility.await().untilAsserted(() -> assertThat(getLatestCatalogVersion(node1), is(catalog2.version() + 1)));
+ Awaitility.await().until(() -> getLatestCatalogVersion(node1), is(catalog2.version() + 1));
Review Comment:
[nitpick] This assertion was changed from `untilAsserted()` to `until()`, but the semantics differ: `until()` returns the supplier's value directly while `untilAsserted()` evaluates the assertion. The new form works but is less explicit about what's being tested. Consider keeping `untilAsserted()` unless there's a specific reason to change it.
```suggestion
Awaitility.await().untilAsserted(() -> assertThat(getLatestCatalogVersion(node1), is(catalog2.version() + 1)));
```
##########
modules/catalog-compaction/src/integrationTest/java/org/apache/ignite/internal/catalog/compaction/ItCatalogCompactionTest.java:
##########
@@ -138,63 +145,83 @@ void testGlobalMinimumTxRequiredTime() {
IgniteImpl node1 = unwrapIgniteImpl(CLUSTER.node(1));
IgniteImpl node2 = unwrapIgniteImpl(CLUSTER.node(2));
+ DebugInfoCollector debug = new DebugInfoCollector(List.of(node0, node1, node2));
+
List<CatalogCompactionRunner> compactors = List.of(
node0.catalogCompactionRunner(),
node1.catalogCompactionRunner(),
node2.catalogCompactionRunner()
);
- Catalog catalog1 = getLatestCatalog(node2);
+ debug.recordGlobalTxState("init");
+ debug.recordCatalogState("init");
+ debug.recordMinTxTimesState("init");
- Transaction tx1 = beginTx(node0, false);
+ Catalog catalog1 = getLatestCatalog(node2);
+ InternalTransaction tx1 = beginTx(node0, false);
+ debug.recordTx(tx1);
// Changing the catalog and starting transaction.
sql("create table a(a int primary key)");
Catalog catalog2 = getLatestCatalog(node0);
assertThat(catalog2.version(), is(catalog1.version() + 1));
- List<Transaction> txs2 = Stream.of(node1, node2).map(node -> beginTx(node, false)).collect(Collectors.toList());
+ List<InternalTransaction> txs2 = Stream.of(node1, node2).map(node -> beginTx(node, false)).collect(Collectors.toList());
List<InternalTransaction> ignoredReadonlyTxs = Stream.of(node0, node1, node2)
.map(node -> beginTx(node, true))
.collect(Collectors.toList());
+ debug.recordTx(txs2);
+ debug.recordTx(ignoredReadonlyTxs);
+
// Changing the catalog again and starting transaction.
sql("alter table a add column (b int)");
- Awaitility.await().untilAsserted(() -> assertThat(getLatestCatalogVersion(node1), is(catalog2.version() + 1)));
+ Awaitility.await().until(() -> getLatestCatalogVersion(node1), is(catalog2.version() + 1));
Catalog catalog3 = getLatestCatalog(node1);
- List<Transaction> txs3 = Stream.of(node0, node2).map(node -> beginTx(node, false)).collect(Collectors.toList());
+ List<InternalTransaction> txs3 = Stream.of(node0, node2).map(node -> beginTx(node, false)).collect(Collectors.toList());
+
+ debug.recordTx(txs3);
Collection<InternalClusterNode> topologyNodes = node0.cluster().nodes().stream()
.map(ClusterNodeImpl::fromPublicClusterNode)
.collect(toUnmodifiableList());
- compactors.forEach(compactor -> {
- TimeHolder timeHolder = await(compactor.determineGlobalMinimumRequiredTime(topologyNodes, 0L));
- assertThat(timeHolder.txMinRequiredTime, is(catalog1.time()));
- });
+ for (int i = 0; i < compactors.size(); i++) {
+ TimeHolder timeHolder = await(compactors.get(i).determineGlobalMinimumRequiredTime(topologyNodes, 0L));
+
+ String failureMessage = "Initial condition failed on node #" + i;
+
+ assertEquals(catalog1.time(), timeHolder.txMinRequiredTime, () -> debug.dumpDebugInfo(failureMessage));
+ }
tx1.rollback();
- compactors.forEach(compactor -> {
- TimeHolder timeHolder = await(compactor.determineGlobalMinimumRequiredTime(topologyNodes, 0L));
- assertThat(timeHolder.txMinRequiredTime, is(catalog2.time()));
- });
+ for (int i = 0; i < compactors.size(); i++) {
+ TimeHolder timeHolder = await(compactors.get(i).determineGlobalMinimumRequiredTime(topologyNodes, 0L));
+
+ String failureMessage = "Condition failed after first tx rollback on node #" + i;
+
+ assertEquals(catalog2.time(), timeHolder.txMinRequiredTime, () -> debug.dumpDebugInfo(failureMessage));
+ }
txs2.forEach(Transaction::commit);
- compactors.forEach(compactor -> {
- TimeHolder timeHolder = await(compactor.determineGlobalMinimumRequiredTime(topologyNodes, 0L));
- assertThat(timeHolder.txMinRequiredTime, is(catalog3.time()));
- });
+ for (int i = 0; i < compactors.size(); i++) {
+ TimeHolder timeHolder = await(compactors.get(i).determineGlobalMinimumRequiredTime(topologyNodes, 0L));
+
+ String failureMessage = "Condition failed after transactions commit on node #" + i;
+
+ assertEquals(catalog3.time(), timeHolder.txMinRequiredTime, () -> debug.dumpDebugInfo(failureMessage));
+ }
txs3.forEach(Transaction::rollback);
// Since there are no active RW transactions in the cluster, the minimum time will be min(now()) across all nodes.
- compactors.forEach(compactor -> {
+ for (int i = 0; i < compactors.size(); i++) {
long minTime = Stream.of(node0, node1, node2).map(node -> node.clockService().nowLong()).min(Long::compareTo).orElseThrow();
- TimeHolder timeHolder = await(compactor.determineGlobalMinimumRequiredTime(topologyNodes, 0L));
+ TimeHolder timeHolder = await(compactors.get(i).determineGlobalMinimumRequiredTime(topologyNodes, 0L));
long maxTime = Stream.of(node0, node1, node2).map(node -> node.clockService().nowLong()).min(Long::compareTo).orElseThrow();
Review Comment:
This should use `.max(Long::compareTo)` instead of `.min(Long::compareTo)` to correctly compute the maximum time across nodes. Currently this is computing the minimum value again, making the assertion range invalid.
```suggestion
long maxTime = Stream.of(node0, node1, node2).map(node -> node.clockService().nowLong()).max(Long::compareTo).orElseThrow();
```
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-26626 Added debug information output to the test (testGlobalMinimumTxRequiredTime) [ignite-3]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #6897:
URL: https://github.com/apache/ignite-3/pull/6897#discussion_r2491692980
##########
modules/catalog-compaction/src/integrationTest/java/org/apache/ignite/internal/catalog/compaction/ItCatalogCompactionTest.java:
##########
@@ -138,63 +144,82 @@ void testGlobalMinimumTxRequiredTime() {
IgniteImpl node1 = unwrapIgniteImpl(CLUSTER.node(1));
IgniteImpl node2 = unwrapIgniteImpl(CLUSTER.node(2));
+ DebugInfoCollector debug = new DebugInfoCollector(List.of(node0, node1, node2));
+
List<CatalogCompactionRunner> compactors = List.of(
node0.catalogCompactionRunner(),
node1.catalogCompactionRunner(),
node2.catalogCompactionRunner()
);
- Catalog catalog1 = getLatestCatalog(node2);
+ debug.recordCatalogState("init");
+ debug.recordMinTxTimesState("init");
- Transaction tx1 = beginTx(node0, false);
+ Catalog catalog1 = getLatestCatalog(node2);
+ InternalTransaction tx1 = beginTx(node0, false);
+ debug.recordTx(tx1);
// Changing the catalog and starting transaction.
sql("create table a(a int primary key)");
Catalog catalog2 = getLatestCatalog(node0);
assertThat(catalog2.version(), is(catalog1.version() + 1));
- List<Transaction> txs2 = Stream.of(node1, node2).map(node -> beginTx(node, false)).collect(Collectors.toList());
+ List<InternalTransaction> txs2 = Stream.of(node1, node2).map(node -> beginTx(node, false)).collect(Collectors.toList());
List<InternalTransaction> ignoredReadonlyTxs = Stream.of(node0, node1, node2)
.map(node -> beginTx(node, true))
.collect(Collectors.toList());
+ debug.recordTx(txs2);
+ debug.recordTx(ignoredReadonlyTxs);
+
// Changing the catalog again and starting transaction.
sql("alter table a add column (b int)");
- Awaitility.await().untilAsserted(() -> assertThat(getLatestCatalogVersion(node1), is(catalog2.version() + 1)));
+ Awaitility.await().until(() -> getLatestCatalogVersion(node1), is(catalog2.version() + 1));
Review Comment:
[nitpick] The changed assertion pattern using `Awaitility.await().until()` with a lambda and matcher is less readable than the original `untilAsserted()` approach. Consider keeping `untilAsserted(() -> assertThat(getLatestCatalogVersion(node1), is(catalog2.version() + 1)))` for consistency with other test patterns and better readability.
```suggestion
Awaitility.await().untilAsserted(() -> assertThat(getLatestCatalogVersion(node1), is(catalog2.version() + 1)));
```
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-26626 Added debug information output to the test (testGlobalMinimumTxRequiredTime) [ignite-3]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #6897:
URL: https://github.com/apache/ignite-3/pull/6897#discussion_r2491692980
##########
modules/catalog-compaction/src/integrationTest/java/org/apache/ignite/internal/catalog/compaction/ItCatalogCompactionTest.java:
##########
@@ -138,63 +144,82 @@ void testGlobalMinimumTxRequiredTime() {
IgniteImpl node1 = unwrapIgniteImpl(CLUSTER.node(1));
IgniteImpl node2 = unwrapIgniteImpl(CLUSTER.node(2));
+ DebugInfoCollector debug = new DebugInfoCollector(List.of(node0, node1, node2));
+
List<CatalogCompactionRunner> compactors = List.of(
node0.catalogCompactionRunner(),
node1.catalogCompactionRunner(),
node2.catalogCompactionRunner()
);
- Catalog catalog1 = getLatestCatalog(node2);
+ debug.recordCatalogState("init");
+ debug.recordMinTxTimesState("init");
- Transaction tx1 = beginTx(node0, false);
+ Catalog catalog1 = getLatestCatalog(node2);
+ InternalTransaction tx1 = beginTx(node0, false);
+ debug.recordTx(tx1);
// Changing the catalog and starting transaction.
sql("create table a(a int primary key)");
Catalog catalog2 = getLatestCatalog(node0);
assertThat(catalog2.version(), is(catalog1.version() + 1));
- List<Transaction> txs2 = Stream.of(node1, node2).map(node -> beginTx(node, false)).collect(Collectors.toList());
+ List<InternalTransaction> txs2 = Stream.of(node1, node2).map(node -> beginTx(node, false)).collect(Collectors.toList());
List<InternalTransaction> ignoredReadonlyTxs = Stream.of(node0, node1, node2)
.map(node -> beginTx(node, true))
.collect(Collectors.toList());
+ debug.recordTx(txs2);
+ debug.recordTx(ignoredReadonlyTxs);
+
// Changing the catalog again and starting transaction.
sql("alter table a add column (b int)");
- Awaitility.await().untilAsserted(() -> assertThat(getLatestCatalogVersion(node1), is(catalog2.version() + 1)));
+ Awaitility.await().until(() -> getLatestCatalogVersion(node1), is(catalog2.version() + 1));
Review Comment:
[nitpick] The changed assertion pattern using `Awaitility.await().until()` with a lambda and matcher is less readable than the original `untilAsserted()` approach. Consider keeping `untilAsserted(() -> assertThat(getLatestCatalogVersion(node1), is(catalog2.version() + 1)))` for consistency with other test patterns and better readability.
```suggestion
Awaitility.await().untilAsserted(() -> assertThat(getLatestCatalogVersion(node1), is(catalog2.version() + 1)));
```
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org