rpuch commented on code in PR #4987:
URL: https://github.com/apache/ignite-3/pull/4987#discussion_r1899490887
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/server/impl/JraftServerImpl.java:
##########
@@ -113,6 +114,9 @@
public class JraftServerImpl implements RaftServer {
private static final IgniteLogger LOG =
Loggers.forClass(JraftServerImpl.class);
+ /** Prefix to save destroy storage intents in {@link
DestroyStorageIntentStorage}. */
+ private static final String DESTROY_PREFIX = "jraftServer";
Review Comment:
```suggestion
private static final String META_AND_SNAPSHOT_DESTROY_INTENT =
"metaAndSnapshot";
```
Because it is about destroying Raft meta and snapshot storages
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/server/impl/JraftServerImpl.java:
##########
@@ -884,4 +898,14 @@ public void onLeaderStop(Status status) {
listener.onLeaderStop();
}
}
+
+ private void completeStoragesDestroy() {
Review Comment:
```suggestion
private void completeStoragesDestruction() {
```
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/server/impl/JraftServerImpl.java:
##########
@@ -573,15 +584,18 @@ public boolean stopRaftNodes(ReplicationGroupId groupId) {
@Override
public void destroyRaftNodeStorages(RaftNodeId nodeId, RaftGroupOptions
groupOptions) {
- // TODO: IGNITE-23079 - improve on what we do if it was not possible
to destroy any of the storages.
try {
String logUri = nodeId.nodeIdStringForStorage();
groupOptions.getLogStorageFactory().destroyLogStorage(logUri);
} finally {
Path serverDataPath = serverDataPathForNodeId(nodeId,
groupOptions);
+
destroyStorageIntentStorage.saveDestroyStorageIntent(DESTROY_PREFIX,
serverDataPath.toString());
+
// This destroys both meta storage and snapshots storage as they
are stored under serverDataPath.
- IgniteUtils.deleteIfExists(serverDataPath);
+ if (IgniteUtils.deleteIfExists(serverDataPath)) {
+
destroyStorageIntentStorage.removeDestroyStorageIntent(DESTROY_PREFIX,
serverDataPath.toString());
+ }
Review Comment:
Let's throw an exception in `else` to see the failure early
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/storage/PersistentLogStorageFactory.java:
##########
@@ -0,0 +1,11 @@
+package org.apache.ignite.internal.raft.storage;
+
+import org.apache.ignite.internal.components.LogSyncer;
+import org.apache.ignite.raft.jraft.storage.DestroyStorageIntentStorage;
+
+/** Persistent log storage factory. */
+// TODO https://issues.apache.org/jira/browse/IGNITE-22766.
+public interface PersistentLogStorageFactory extends LogStorageFactory,
LogSyncer {
+ /** Destroys "leftover" storages saved to {@link
DestroyStorageIntentStorage}. Must be called on factory start. */
+ void completeLogStoragesDestruction();
Review Comment:
This method is only used internally as a private method, it's not needed on
the interface, as it seems
##########
modules/raft/src/main/java/org/apache/ignite/raft/jraft/storage/impl/VaultDestroyStorageIntentStorage.java:
##########
@@ -0,0 +1,82 @@
+package org.apache.ignite.raft.jraft.storage.impl;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.apache.ignite.raft.jraft.util.BytesUtil.EMPTY_BYTES;
+
+import java.nio.ByteBuffer;
+import java.nio.ByteOrder;
+import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+import org.apache.ignite.internal.lang.ByteArray;
+import org.apache.ignite.internal.util.Cursor;
+import org.apache.ignite.internal.vault.VaultEntry;
+import org.apache.ignite.internal.vault.VaultManager;
+import org.apache.ignite.raft.jraft.storage.DestroyStorageIntentStorage;
+
+/** Uses VaultManager to persist and retrieve log storage destroy intents. */
+public class VaultDestroyStorageIntentStorage implements
DestroyStorageIntentStorage {
+ private static final ByteArray LOG_STORAGES_PREFIX = new
ByteArray("destroy.logStorages.");
Review Comment:
```suggestion
private static final ByteArray GROUP_STORAGES_DESTRUCTION_PREFIX = new
ByteArray("destroy.group.storages.");
```
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/server/impl/JraftServerImpl.java:
##########
@@ -884,4 +898,14 @@ public void onLeaderStop(Status status) {
listener.onLeaderStop();
}
}
+
+ private void completeStoragesDestroy() {
+
destroyStorageIntentStorage.storagesToDestroy(DESTROY_PREFIX).forEach(serverDataPath
-> {
+ if (IgniteUtils.deleteIfExists(Path.of(serverDataPath))) {
+
destroyStorageIntentStorage.removeDestroyStorageIntent(DESTROY_PREFIX,
serverDataPath);
+ } else {
+ LOG.error("Failed to delete storage: " + serverDataPath);
Review Comment:
Let's just throw an exception
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/server/impl/JraftServerImpl.java:
##########
@@ -573,15 +584,18 @@ public boolean stopRaftNodes(ReplicationGroupId groupId) {
@Override
public void destroyRaftNodeStorages(RaftNodeId nodeId, RaftGroupOptions
groupOptions) {
- // TODO: IGNITE-23079 - improve on what we do if it was not possible
to destroy any of the storages.
try {
Review Comment:
Both intents should be saved in the very beginning, and this should be done
atomically, in one operation. Please use something like `vault.putAll()` under
the hood
##########
modules/raft/src/main/java/org/apache/ignite/raft/jraft/storage/impl/VaultDestroyStorageIntentStorage.java:
##########
@@ -0,0 +1,82 @@
+package org.apache.ignite.raft.jraft.storage.impl;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.apache.ignite.raft.jraft.util.BytesUtil.EMPTY_BYTES;
+
+import java.nio.ByteBuffer;
+import java.nio.ByteOrder;
+import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+import org.apache.ignite.internal.lang.ByteArray;
+import org.apache.ignite.internal.util.Cursor;
+import org.apache.ignite.internal.vault.VaultEntry;
+import org.apache.ignite.internal.vault.VaultManager;
+import org.apache.ignite.raft.jraft.storage.DestroyStorageIntentStorage;
+
+/** Uses VaultManager to persist and retrieve log storage destroy intents. */
+public class VaultDestroyStorageIntentStorage implements
DestroyStorageIntentStorage {
+ private static final ByteArray LOG_STORAGES_PREFIX = new
ByteArray("destroy.logStorages.");
+ private static final ByteOrder BYTE_UTILS_BYTE_ORDER =
ByteOrder.BIG_ENDIAN;
+
+ private final VaultManager vault;
+
+ /** Constructor. */
+ public VaultDestroyStorageIntentStorage(VaultManager vault) {
+ this.vault = vault;
+ }
+
+ @Override
+ public Collection<String> storagesToDestroy(String storagePrefix) {
+ ByteArray prefix = prefixByFactoryName(storagePrefix);
+
+ try (Cursor<VaultEntry> cursor = vault.prefix(prefix)) {
+ List<String> result = new ArrayList<>();
+
+ while (cursor.hasNext()) {
+ result.add(uriFromKey(storagePrefix,
cursor.next().key().bytes()));
+ }
+
+ return result;
+ }
+ }
+
+ @Override
+ public void saveDestroyStorageIntent(String storagePrefix, String uri) {
+ vault.put(buildKey(storagePrefix, uri), EMPTY_BYTES);
+ }
+
+ @Override
+ public void removeDestroyStorageIntent(String storagePrefix, String uri) {
+ vault.remove(buildKey(storagePrefix, uri));
+ }
+
+ private static ByteArray buildKey(String storagePrefix, String uri) {
+ byte[] key = ByteBuffer.allocate(LOG_STORAGES_PREFIX.length() +
storagePrefix.length() + uri.length())
+ .order(BYTE_UTILS_BYTE_ORDER)
+ .put(LOG_STORAGES_PREFIX.bytes())
+ .put(storagePrefix.getBytes(UTF_8))
+ .putChar('.')
Review Comment:
1. The dot doesn't seem to be accounted for when calculating buffer fize
2. length() might return less than the size of `getBytes()`
##########
modules/raft/src/main/java/org/apache/ignite/raft/jraft/storage/impl/VaultDestroyStorageIntentStorage.java:
##########
@@ -0,0 +1,82 @@
+package org.apache.ignite.raft.jraft.storage.impl;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.apache.ignite.raft.jraft.util.BytesUtil.EMPTY_BYTES;
+
+import java.nio.ByteBuffer;
+import java.nio.ByteOrder;
+import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+import org.apache.ignite.internal.lang.ByteArray;
+import org.apache.ignite.internal.util.Cursor;
+import org.apache.ignite.internal.vault.VaultEntry;
+import org.apache.ignite.internal.vault.VaultManager;
+import org.apache.ignite.raft.jraft.storage.DestroyStorageIntentStorage;
+
+/** Uses VaultManager to persist and retrieve log storage destroy intents. */
+public class VaultDestroyStorageIntentStorage implements
DestroyStorageIntentStorage {
+ private static final ByteArray LOG_STORAGES_PREFIX = new
ByteArray("destroy.logStorages.");
+ private static final ByteOrder BYTE_UTILS_BYTE_ORDER =
ByteOrder.BIG_ENDIAN;
+
+ private final VaultManager vault;
+
+ /** Constructor. */
+ public VaultDestroyStorageIntentStorage(VaultManager vault) {
+ this.vault = vault;
+ }
+
+ @Override
+ public Collection<String> storagesToDestroy(String storagePrefix) {
+ ByteArray prefix = prefixByFactoryName(storagePrefix);
+
+ try (Cursor<VaultEntry> cursor = vault.prefix(prefix)) {
+ List<String> result = new ArrayList<>();
+
+ while (cursor.hasNext()) {
+ result.add(uriFromKey(storagePrefix,
cursor.next().key().bytes()));
+ }
+
+ return result;
+ }
+ }
+
+ @Override
+ public void saveDestroyStorageIntent(String storagePrefix, String uri) {
+ vault.put(buildKey(storagePrefix, uri), EMPTY_BYTES);
+ }
+
+ @Override
+ public void removeDestroyStorageIntent(String storagePrefix, String uri) {
+ vault.remove(buildKey(storagePrefix, uri));
+ }
+
+ private static ByteArray buildKey(String storagePrefix, String uri) {
+ byte[] key = ByteBuffer.allocate(LOG_STORAGES_PREFIX.length() +
storagePrefix.length() + uri.length())
+ .order(BYTE_UTILS_BYTE_ORDER)
+ .put(LOG_STORAGES_PREFIX.bytes())
+ .put(storagePrefix.getBytes(UTF_8))
+ .putChar('.')
+ .put(uri.getBytes(UTF_8))
+ .array();
+
+ return new ByteArray(key);
+ }
+
+ private static String uriFromKey(String factoryName, byte[] key) {
+ int offset = LOG_STORAGES_PREFIX.length() + factoryName.length() + 1;
+
+ return new String(key, offset, key.length - offset, UTF_8);
+ }
+
+ private static ByteArray prefixByFactoryName(String factoryName) {
Review Comment:
Why is it about factory name? It seems to be an abstract prefix/identifier
(as it now accepts not just factory names, but also server data path constant)
##########
modules/raft/src/main/java/org/apache/ignite/raft/jraft/storage/impl/VaultDestroyStorageIntentStorage.java:
##########
@@ -0,0 +1,82 @@
+package org.apache.ignite.raft.jraft.storage.impl;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.apache.ignite.raft.jraft.util.BytesUtil.EMPTY_BYTES;
+
+import java.nio.ByteBuffer;
+import java.nio.ByteOrder;
+import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+import org.apache.ignite.internal.lang.ByteArray;
+import org.apache.ignite.internal.util.Cursor;
+import org.apache.ignite.internal.vault.VaultEntry;
+import org.apache.ignite.internal.vault.VaultManager;
+import org.apache.ignite.raft.jraft.storage.DestroyStorageIntentStorage;
+
+/** Uses VaultManager to persist and retrieve log storage destroy intents. */
+public class VaultDestroyStorageIntentStorage implements
DestroyStorageIntentStorage {
+ private static final ByteArray LOG_STORAGES_PREFIX = new
ByteArray("destroy.logStorages.");
+ private static final ByteOrder BYTE_UTILS_BYTE_ORDER =
ByteOrder.BIG_ENDIAN;
+
+ private final VaultManager vault;
+
+ /** Constructor. */
+ public VaultDestroyStorageIntentStorage(VaultManager vault) {
+ this.vault = vault;
+ }
+
+ @Override
+ public Collection<String> storagesToDestroy(String storagePrefix) {
+ ByteArray prefix = prefixByFactoryName(storagePrefix);
+
+ try (Cursor<VaultEntry> cursor = vault.prefix(prefix)) {
+ List<String> result = new ArrayList<>();
+
+ while (cursor.hasNext()) {
+ result.add(uriFromKey(storagePrefix,
cursor.next().key().bytes()));
+ }
+
+ return result;
+ }
+ }
+
+ @Override
+ public void saveDestroyStorageIntent(String storagePrefix, String uri) {
+ vault.put(buildKey(storagePrefix, uri), EMPTY_BYTES);
+ }
+
+ @Override
+ public void removeDestroyStorageIntent(String storagePrefix, String uri) {
+ vault.remove(buildKey(storagePrefix, uri));
+ }
+
+ private static ByteArray buildKey(String storagePrefix, String uri) {
+ byte[] key = ByteBuffer.allocate(LOG_STORAGES_PREFIX.length() +
storagePrefix.length() + uri.length())
+ .order(BYTE_UTILS_BYTE_ORDER)
+ .put(LOG_STORAGES_PREFIX.bytes())
+ .put(storagePrefix.getBytes(UTF_8))
+ .putChar('.')
+ .put(uri.getBytes(UTF_8))
+ .array();
+
+ return new ByteArray(key);
+ }
+
+ private static String uriFromKey(String factoryName, byte[] key) {
+ int offset = LOG_STORAGES_PREFIX.length() + factoryName.length() + 1;
+
+ return new String(key, offset, key.length - offset, UTF_8);
+ }
+
+ private static ByteArray prefixByFactoryName(String factoryName) {
+ byte[] buffer = ByteBuffer.allocate(LOG_STORAGES_PREFIX.length() +
factoryName.length())
+ .order(BYTE_UTILS_BYTE_ORDER)
+ .put(LOG_STORAGES_PREFIX.bytes())
+ .put(factoryName.getBytes(UTF_8))
Review Comment:
`length()` returns length in characters, while `getBytes()` will return
bytes; number of bytes might be more than number of characters
##########
modules/vault/src/main/java/org/apache/ignite/internal/vault/VaultManager.java:
##########
@@ -102,6 +103,14 @@ public Cursor<VaultEntry> range(ByteArray fromKey,
ByteArray toKey) {
return vaultSvc.range(fromKey, toKey);
}
+ /**
+ * @param prefix Prefix to get entries by. Cannot be {@code null}.
Review Comment:
Method description is missing
##########
modules/raft/src/main/java/org/apache/ignite/raft/jraft/storage/DestroyStorageIntentStorage.java:
##########
@@ -0,0 +1,15 @@
+package org.apache.ignite.raft.jraft.storage;
+
+import java.util.Collection;
+
+/** Persists and retrieves intent to complete storages destruction on node
start. */
+public interface DestroyStorageIntentStorage extends Storage {
Review Comment:
```suggestion
public interface GroupStoragesDestructionIntents extends Storage {
```
Also:
1. I don't think it should reside in jraft package
2. I double it has to extend `Storage`
##########
modules/raft/src/main/java/org/apache/ignite/raft/jraft/storage/impl/VaultDestroyStorageIntentStorage.java:
##########
@@ -0,0 +1,82 @@
+package org.apache.ignite.raft.jraft.storage.impl;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.apache.ignite.raft.jraft.util.BytesUtil.EMPTY_BYTES;
+
+import java.nio.ByteBuffer;
+import java.nio.ByteOrder;
+import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+import org.apache.ignite.internal.lang.ByteArray;
+import org.apache.ignite.internal.util.Cursor;
+import org.apache.ignite.internal.vault.VaultEntry;
+import org.apache.ignite.internal.vault.VaultManager;
+import org.apache.ignite.raft.jraft.storage.DestroyStorageIntentStorage;
+
+/** Uses VaultManager to persist and retrieve log storage destroy intents. */
Review Comment:
```suggestion
/** Uses VaultManager to persist and retrieve group storages destruction
intents. */
```
--
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
