Copilot commented on code in PR #13773:
URL: https://github.com/apache/ignite/pull/13773#discussion_r4424165420
##########
modules/core/src/test/java/org/apache/ignite/internal/processors/continuous/ContinuousQueryBuffersCleanupTest.java:
##########
@@ -146,6 +148,33 @@ public void testContinuousQueryBuffersCleanup() throws
Exception {
}
}
+ /** Checks that a backup acknowledgement covering several topology
versions is sent to all backups. */
Review Comment:
The PR description currently contains only the contribution checklist
template and does not describe the WHAT/WHY of the change. Please
replace/update the PR body to briefly explain the underlying bug (ack not
delivered to all backups when spanning topology versions) and how this PR fixes
it (node de-dup keyed by destination node ID + regression test).
##########
modules/core/src/test/java/org/apache/ignite/internal/processors/continuous/ContinuousQueryBuffersCleanupTest.java:
##########
@@ -146,6 +148,33 @@ public void testContinuousQueryBuffersCleanup() throws
Exception {
}
}
+ /** Checks that a backup acknowledgement covering several topology
versions is sent to all backups. */
+ @Test
+ public void testBackupAcknowledgeCoveringSeveralTopologyVersions() throws
Exception {
+ startGrids(3);
+
+ // Keys of the first node are in one partition, so backup CQ buffer
exists before the acknowledgement.
+ IgniteCache<Object, Object> cache = grid(0).createCache(new
CacheConfiguration<>(DEFAULT_CACHE_NAME)
+ .setAffinity(new GridCacheModuloAffinityFunction(3, 2))
+ .setWriteSynchronizationMode(FULL_SYNC)
+ .setBackups(2));
+
+ // Disable acknowledgement by timeout, so the single acknowledgement
covers both topology versions.
+
GridTestUtils.<Closeable>getFieldValue(grid(0).cachex(DEFAULT_CACHE_NAME).context().continuousQueries(),
+ "cancelableTask").close();
+
Review Comment:
Closing `cancelableTask` via reflective access is brittle (field name
coupling) and can make failures harder to diagnose if internals change. If
reflection is unavoidable here, consider (a) asserting the field value is
non-null and (b) restoring/restarting the task in a `finally` block (or using a
dedicated test hook/configuration if available) to avoid impacting subsequent
test logic on the same node lifecycle.
##########
modules/core/src/test/java/org/apache/ignite/internal/processors/continuous/ContinuousQueryBuffersCleanupTest.java:
##########
@@ -146,6 +148,33 @@ public void testContinuousQueryBuffersCleanup() throws
Exception {
}
}
+ /** Checks that a backup acknowledgement covering several topology
versions is sent to all backups. */
+ @Test
+ public void testBackupAcknowledgeCoveringSeveralTopologyVersions() throws
Exception {
+ startGrids(3);
+
+ // Keys of the first node are in one partition, so backup CQ buffer
exists before the acknowledgement.
+ IgniteCache<Object, Object> cache = grid(0).createCache(new
CacheConfiguration<>(DEFAULT_CACHE_NAME)
+ .setAffinity(new GridCacheModuloAffinityFunction(3, 2))
+ .setWriteSynchronizationMode(FULL_SYNC)
+ .setBackups(2));
+
+ // Disable acknowledgement by timeout, so the single acknowledgement
covers both topology versions.
+
GridTestUtils.<Closeable>getFieldValue(grid(0).cachex(DEFAULT_CACHE_NAME).context().continuousQueries(),
+ "cancelableTask").close();
+
+ try (QueryCursor<?> ignored = cache.query(new
ContinuousQuery<>().setLocalListener(evts -> {}))) {
+ for (int i = 0; i < DFLT_CONTINUOUS_QUERY_BACKUP_ACK_THRESHOLD;
i++) {
+ if (i == DFLT_CONTINUOUS_QUERY_BACKUP_ACK_THRESHOLD / 2)
+ startClientGrid(3);
+
Review Comment:
This test intends to ensure a single backup-ack covers multiple topology
versions, but after `startClientGrid(3)` it immediately continues
`cache.put(...)` without waiting for the topology/PME to complete. That can
make the test flaky (the subsequent puts may still execute under the old
topology version). Consider awaiting partition map exchange (or otherwise
waiting for the new topology version to be visible) right after starting the
client grid, and optionally asserting that the topology version actually
advanced before continuing.
--
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]
