tkalkirill commented on code in PR #4931:
URL: https://github.com/apache/ignite-3/pull/4931#discussion_r1891442422
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/impl/MetaStorageService.java:
##########
@@ -134,6 +134,17 @@ public interface MetaStorageService extends
ManuallyCloseable {
*/
CompletableFuture<Void> removeAll(Set<ByteArray> keys);
+ /**
+ * Removes entries by given prefix.
+ *
+ * @param prefix Prefix to remove keys by. Couldn't be {@code null}.
+ * @return Completed future.
+ * @throws OperationTimeoutException If the operation is timed out. Will
be thrown on getting future result.
+ * @see ByteArray
+ * @see Entry
+ */
+ CompletableFuture<Void> removeAll(ByteArray prefix);
Review Comment:
same about rename
##########
modules/metastorage-api/src/main/java/org/apache/ignite/internal/metastorage/MetaStorageManager.java:
##########
@@ -304,6 +304,11 @@ public interface MetaStorageManager extends
IgniteComponent {
*/
CompletableFuture<Void> removeAll(Set<ByteArray> keys);
+ /**
+ * Removes entries by the given prefix.
+ */
+ CompletableFuture<Void> removeAll(ByteArray prefix);
Review Comment:
I suggest renaming it to `removeByPrefix` , this name seems more suitable to
me.
##########
modules/metastorage/src/test/java/org/apache/ignite/internal/metastorage/server/BasicOperationsKeyValueStorageTest.java:
##########
@@ -638,6 +638,77 @@ public void removeAll() {
assertTrue(e4.empty());
}
+ @Test
+ public void removeAllByPrefix() {
+ byte[] key1 = key(1);
+ byte[] val1 = keyValue(1, 1);
+
+ byte[] key2 = key(2);
+ byte[] val21 = keyValue(2, 21);
+ byte[] val22 = keyValue(2, 22);
+
+ byte[] key3 = key(3);
+ byte[] val31 = keyValue(3, 31);
+
+ byte[] key4 = key(4);
+
+ assertEquals(0, storage.revision());
+
+ // Regular put.
+ putToMs(key1, val1);
+
+ // Rewrite.
+ putToMs(key2, val21);
+ putToMs(key2, val22);
+
+ // Remove. Tombstone must not be removed again.
+ putToMs(key3, val31);
+ removeFromMs(key3);
+
+ assertEquals(5, storage.revision());
+
+ removeAllFromMs(PREFIX_BYTES);
+
+ assertEquals(6, storage.revision());
Review Comment:
same about revision check
##########
modules/metastorage/src/test/java/org/apache/ignite/internal/metastorage/server/BasicOperationsKeyValueStorageTest.java:
##########
@@ -638,6 +638,77 @@ public void removeAll() {
assertTrue(e4.empty());
}
+ @Test
+ public void removeAllByPrefix() {
+ byte[] key1 = key(1);
+ byte[] val1 = keyValue(1, 1);
+
+ byte[] key2 = key(2);
+ byte[] val21 = keyValue(2, 21);
+ byte[] val22 = keyValue(2, 22);
+
+ byte[] key3 = key(3);
+ byte[] val31 = keyValue(3, 31);
+
+ byte[] key4 = key(4);
+
+ assertEquals(0, storage.revision());
+
+ // Regular put.
+ putToMs(key1, val1);
+
+ // Rewrite.
+ putToMs(key2, val21);
+ putToMs(key2, val22);
+
+ // Remove. Tombstone must not be removed again.
+ putToMs(key3, val31);
+ removeFromMs(key3);
+
+ assertEquals(5, storage.revision());
Review Comment:
same about revision check
##########
modules/metastorage/src/test/java/org/apache/ignite/internal/metastorage/server/BasicOperationsKeyValueStorageTest.java:
##########
@@ -638,6 +638,77 @@ public void removeAll() {
assertTrue(e4.empty());
}
+ @Test
+ public void removeAllByPrefix() {
+ byte[] key1 = key(1);
+ byte[] val1 = keyValue(1, 1);
+
+ byte[] key2 = key(2);
+ byte[] val21 = keyValue(2, 21);
+ byte[] val22 = keyValue(2, 22);
+
+ byte[] key3 = key(3);
+ byte[] val31 = keyValue(3, 31);
+
+ byte[] key4 = key(4);
+
+ assertEquals(0, storage.revision());
+
+ // Regular put.
+ putToMs(key1, val1);
+
+ // Rewrite.
+ putToMs(key2, val21);
+ putToMs(key2, val22);
+
+ // Remove. Tombstone must not be removed again.
+ putToMs(key3, val31);
+ removeFromMs(key3);
+
+ assertEquals(5, storage.revision());
+
+ removeAllFromMs(PREFIX_BYTES);
+
+ assertEquals(6, storage.revision());
+
+ Collection<Entry> entries = storage.getAll(List.of(key1, key2, key3,
key4));
+
+ assertEquals(4, entries.size());
+
+ Map<ByteArray, Entry> map =
entries.stream().collect(Collectors.toMap(e -> new ByteArray(e.key()),
identity()));
+
+ // Test regular put value.
+ Entry e1 = map.get(new ByteArray(key1));
+
+ assertNotNull(e1);
+ assertEquals(6, e1.revision());
+ assertTrue(e1.tombstone());
+ assertFalse(e1.empty());
+
+ // Test rewritten value.
+ Entry e2 = map.get(new ByteArray(key2));
+
+ assertNotNull(e2);
+ assertEquals(6, e2.revision());
Review Comment:
same about revision check
##########
modules/metastorage/src/testFixtures/java/org/apache/ignite/internal/metastorage/server/SimpleInMemoryKeyValueStorage.java:
##########
@@ -285,6 +285,33 @@ public void removeAll(List<byte[]> keys,
KeyValueUpdateContext context) {
}
}
+ @Override
+ public void removeAll(byte[] prefix, KeyValueUpdateContext context) {
+ rwLock.writeLock().lock();
+
+ try (Cursor<Entry> cursor = range(prefix, nextKey(prefix))) {
+ long curRev = rev + 1;
+
+ List<byte[]> keys = new ArrayList<>();
+
+ List<byte[]> vals = new ArrayList<>();
Review Comment:
same about var keyword
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/impl/MetaStorageService.java:
##########
@@ -134,6 +134,17 @@ public interface MetaStorageService extends
ManuallyCloseable {
*/
CompletableFuture<Void> removeAll(Set<ByteArray> keys);
+ /**
+ * Removes entries by given prefix.
+ *
+ * @param prefix Prefix to remove keys by. Couldn't be {@code null}.
+ * @return Completed future.
Review Comment:
Why return the future if it will be completed, I think it is worth changing
the description or logic.
##########
modules/metastorage/src/test/java/org/apache/ignite/internal/metastorage/server/BasicOperationsKeyValueStorageTest.java:
##########
@@ -638,6 +638,77 @@ public void removeAll() {
assertTrue(e4.empty());
}
+ @Test
+ public void removeAllByPrefix() {
Review Comment:
same about rename
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/impl/MetaStorageManagerImpl.java:
##########
@@ -967,6 +967,24 @@ public CompletableFuture<Void> removeAll(Set<ByteArray>
keys) {
}
}
+ /**
Review Comment:
I think the comment here is redundant, let's get rid of it.
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/server/KeyValueStorage.java:
##########
@@ -283,6 +283,14 @@ public interface KeyValueStorage extends ManuallyCloseable
{
*/
void removeAll(List<byte[]> keys, KeyValueUpdateContext context);
+ /**
+ * Remove all entries corresponding to given prefix.
+ *
+ * @param prefix Prefix.
+ * @param context Operation's context.
+ */
+ void removeAll(byte[] prefix, KeyValueUpdateContext context);
Review Comment:
same about rename
##########
modules/metastorage/src/test/java/org/apache/ignite/internal/metastorage/server/BasicOperationsKeyValueStorageTest.java:
##########
@@ -2295,6 +2366,10 @@ void removeAllFromMs(List<byte[]> keys) {
storage.removeAll(keys, kvContext(MIN_VALUE));
}
+ void removeAllFromMs(byte[] prefix) {
Review Comment:
same about rename
##########
modules/metastorage/src/test/java/org/apache/ignite/internal/metastorage/server/BasicOperationsKeyValueStorageTest.java:
##########
@@ -638,6 +638,77 @@ public void removeAll() {
assertTrue(e4.empty());
}
+ @Test
+ public void removeAllByPrefix() {
+ byte[] key1 = key(1);
+ byte[] val1 = keyValue(1, 1);
+
+ byte[] key2 = key(2);
+ byte[] val21 = keyValue(2, 21);
+ byte[] val22 = keyValue(2, 22);
+
+ byte[] key3 = key(3);
+ byte[] val31 = keyValue(3, 31);
+
+ byte[] key4 = key(4);
+
+ assertEquals(0, storage.revision());
Review Comment:
Please tell me why we need such a check and is it really necessary?
I assume that such checks should have already been present.
##########
modules/metastorage/src/test/java/org/apache/ignite/internal/metastorage/server/BasicOperationsKeyValueStorageTest.java:
##########
@@ -638,6 +638,77 @@ public void removeAll() {
assertTrue(e4.empty());
}
+ @Test
+ public void removeAllByPrefix() {
+ byte[] key1 = key(1);
+ byte[] val1 = keyValue(1, 1);
+
+ byte[] key2 = key(2);
+ byte[] val21 = keyValue(2, 21);
+ byte[] val22 = keyValue(2, 22);
+
+ byte[] key3 = key(3);
+ byte[] val31 = keyValue(3, 31);
+
+ byte[] key4 = key(4);
+
+ assertEquals(0, storage.revision());
+
+ // Regular put.
+ putToMs(key1, val1);
+
+ // Rewrite.
+ putToMs(key2, val21);
+ putToMs(key2, val22);
+
+ // Remove. Tombstone must not be removed again.
+ putToMs(key3, val31);
+ removeFromMs(key3);
+
+ assertEquals(5, storage.revision());
+
+ removeAllFromMs(PREFIX_BYTES);
+
+ assertEquals(6, storage.revision());
+
+ Collection<Entry> entries = storage.getAll(List.of(key1, key2, key3,
key4));
+
+ assertEquals(4, entries.size());
+
+ Map<ByteArray, Entry> map =
entries.stream().collect(Collectors.toMap(e -> new ByteArray(e.key()),
identity()));
+
+ // Test regular put value.
+ Entry e1 = map.get(new ByteArray(key1));
+
+ assertNotNull(e1);
+ assertEquals(6, e1.revision());
+ assertTrue(e1.tombstone());
+ assertFalse(e1.empty());
+
+ // Test rewritten value.
+ Entry e2 = map.get(new ByteArray(key2));
+
+ assertNotNull(e2);
+ assertEquals(6, e2.revision());
+ assertTrue(e2.tombstone());
+ assertFalse(e2.empty());
+
+ // Test removed value.
+ Entry e3 = map.get(new ByteArray(key3));
+
+ assertNotNull(e3);
+ assertEquals(5, e3.revision());
Review Comment:
same about revision check
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/server/persistence/RocksDbKeyValueStorage.java:
##########
@@ -783,6 +783,35 @@ public void removeAll(List<byte[]> keys,
KeyValueUpdateContext context) {
}
}
+ @Override
+ public void removeAll(byte[] prefix, KeyValueUpdateContext context) {
+ rwLock.writeLock().lock();
+
+ try (WriteBatch batch = new WriteBatch(); Cursor<Entry> entries =
range(prefix, nextKey(prefix))) {
+ long curRev = rev + 1;
+
+ List<byte[]> keys = new ArrayList<>();
Review Comment:
I think that using a collection here can be memory intensive and I think
that we can delete immediately by cursor.
##########
modules/metastorage/src/test/java/org/apache/ignite/internal/metastorage/server/BasicOperationsKeyValueStorageTest.java:
##########
@@ -638,6 +638,77 @@ public void removeAll() {
assertTrue(e4.empty());
}
+ @Test
+ public void removeAllByPrefix() {
+ byte[] key1 = key(1);
+ byte[] val1 = keyValue(1, 1);
+
+ byte[] key2 = key(2);
+ byte[] val21 = keyValue(2, 21);
+ byte[] val22 = keyValue(2, 22);
+
+ byte[] key3 = key(3);
+ byte[] val31 = keyValue(3, 31);
+
+ byte[] key4 = key(4);
+
+ assertEquals(0, storage.revision());
+
+ // Regular put.
+ putToMs(key1, val1);
+
+ // Rewrite.
+ putToMs(key2, val21);
+ putToMs(key2, val22);
+
+ // Remove. Tombstone must not be removed again.
+ putToMs(key3, val31);
+ removeFromMs(key3);
+
+ assertEquals(5, storage.revision());
+
+ removeAllFromMs(PREFIX_BYTES);
+
+ assertEquals(6, storage.revision());
+
+ Collection<Entry> entries = storage.getAll(List.of(key1, key2, key3,
key4));
+
+ assertEquals(4, entries.size());
+
+ Map<ByteArray, Entry> map =
entries.stream().collect(Collectors.toMap(e -> new ByteArray(e.key()),
identity()));
+
+ // Test regular put value.
+ Entry e1 = map.get(new ByteArray(key1));
+
+ assertNotNull(e1);
+ assertEquals(6, e1.revision());
Review Comment:
I think it would be better to check from the latest revision instead of the
absolute value.
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/server/persistence/RocksDbKeyValueStorage.java:
##########
@@ -783,6 +783,35 @@ public void removeAll(List<byte[]> keys,
KeyValueUpdateContext context) {
}
}
+ @Override
+ public void removeAll(byte[] prefix, KeyValueUpdateContext context) {
+ rwLock.writeLock().lock();
+
+ try (WriteBatch batch = new WriteBatch(); Cursor<Entry> entries =
range(prefix, nextKey(prefix))) {
+ long curRev = rev + 1;
+
+ List<byte[]> keys = new ArrayList<>();
Review Comment:
u can use var keyword
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/server/persistence/RocksDbKeyValueStorage.java:
##########
@@ -783,6 +783,35 @@ public void removeAll(List<byte[]> keys,
KeyValueUpdateContext context) {
}
}
+ @Override
+ public void removeAll(byte[] prefix, KeyValueUpdateContext context) {
+ rwLock.writeLock().lock();
+
+ try (WriteBatch batch = new WriteBatch(); Cursor<Entry> entries =
range(prefix, nextKey(prefix))) {
Review Comment:
lets use entryCursor
##########
modules/metastorage/src/testFixtures/java/org/apache/ignite/internal/metastorage/server/SimpleInMemoryKeyValueStorage.java:
##########
@@ -285,6 +285,33 @@ public void removeAll(List<byte[]> keys,
KeyValueUpdateContext context) {
}
}
+ @Override
+ public void removeAll(byte[] prefix, KeyValueUpdateContext context) {
+ rwLock.writeLock().lock();
+
+ try (Cursor<Entry> cursor = range(prefix, nextKey(prefix))) {
+ long curRev = rev + 1;
+
+ List<byte[]> keys = new ArrayList<>();
Review Comment:
u can use var keyword
##########
modules/metastorage/src/test/java/org/apache/ignite/internal/metastorage/server/BasicOperationsKeyValueStorageTest.java:
##########
@@ -638,6 +638,77 @@ public void removeAll() {
assertTrue(e4.empty());
}
+ @Test
+ public void removeAllByPrefix() {
Review Comment:
Let's add a test that we don't delete adjacent keys with a different prefix.
--
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
