rpuch commented on code in PR #5698:
URL: https://github.com/apache/ignite-3/pull/5698#discussion_r2063368191


##########
modules/metastorage/src/test/java/org/apache/ignite/internal/metastorage/server/AbstractCompactionKeyValueStorageTest.java:
##########
@@ -526,6 +526,55 @@ void testDoNotDeleteOnExactMatchCompaction() {
         assertEquals(entryBefore, entryAfter);
     }
 
+    @Test
+    void testReadCompactedTombstoneConcurrent() {
+        KeyValueUpdateContext context = kvContext(hybridTimestamp(10L));
+
+        byte[] extraKey = key(-1);
+        byte[] extraValue = keyValue(0, 0);
+
+        for (int i = 0; i < 1000; i++) {
+            byte[] key = key(i);
+            byte[] value = keyValue(i, i);
+
+            storage.put(key, value, context);
+            storage.remove(key, context);
+
+            long revision = storage.revision();
+
+            // Increase revision, so that compaction on "revision" can safely 
be executed.
+            storage.put(extraKey, extraValue, context);
+
+            runRace(
+                    () -> {
+                        storage.setCompactionRevision(revision);
+                        storage.compact(revision);
+                    },
+                    () -> {
+                        // We update the same value in order to cause a race 
in "writeBatch" method, that would leave already compacted
+                        // revisions in a list of revisions associated with 
this key.
+                        storage.put(key, value, context);
+                    }
+            );
+
+            // Not a real assertion. It's for a developer that reads this 
test. Yes, you.
+            assertEquals(revision + 2, storage.revision());
+
+            long nextToLast = storage.revision() - 1;
+
+            // Here we read the previous value of the key that's been inserted 
right now.
+            // WatchProcessor does exactly the same thing, and it must work, 
otherwise Ignite nodes will fail in "remove+put" scenarios.
+            Entry entry = storage.get(key, nextToLast);

Review Comment:
   Is it true that this is the central line of the test scenario, and we expect 
that nothing will explode here? If yes, it makes sense to wrap it in 
`assertDoesNotThrow()` and maybe additionally say in the comment that this IS 
the real assertion :)



-- 
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-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Reply via email to