rpuch commented on code in PR #4987: URL: https://github.com/apache/ignite-3/pull/4987#discussion_r1925204109
########## modules/raft/src/integrationTest/java/org/apache/ignite/raft/server/ItJraftDestructorTest.java: ########## @@ -0,0 +1,145 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.raft.server; + +import static org.apache.ignite.internal.testframework.IgniteTestUtils.assertThrows; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Objects; +import java.util.stream.Stream; +import org.apache.ignite.internal.cluster.management.CmgGroupId; +import org.apache.ignite.internal.lang.IgniteInternalException; +import org.apache.ignite.internal.metastorage.server.raft.MetastorageGroupId; +import org.apache.ignite.internal.raft.Peer; +import org.apache.ignite.internal.raft.RaftNodeId; +import org.apache.ignite.internal.raft.server.RaftGroupOptions; +import org.apache.ignite.internal.raft.server.impl.JraftServerImpl; +import org.apache.ignite.internal.raft.storage.LogStorageFactory; +import org.apache.ignite.internal.raft.storage.impl.LogStorageException; +import org.apache.ignite.internal.replicator.ReplicationGroupId; +import org.apache.ignite.internal.replicator.TablePartitionId; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.MethodSource; + +/** Tests that check that failed storage destruction is finished after server restart. */ +public class ItJraftDestructorTest extends JraftAbstractTest { + + private static Stream<ReplicationGroupId> groupIdProvider() { + return Stream.of( + new TablePartitionId(0, 0), + CmgGroupId.INSTANCE, + MetastorageGroupId.INSTANCE + ); + } + + private JraftServerImpl server; + private Path serverDataPath; + private Peer peer; + + @BeforeEach + void setUp() { + server = startServer(); + + serverDataPath = serverWorkingDirs.get(0).basePath(); + + String localNodeName = server.clusterService().topologyService().localMember().name(); + + peer = Objects.requireNonNull(initialMembersConf.peer(localNodeName)); + + LogStorageFactory logStorageFactory = logStorageFactories.get(0); + + doThrow(LogStorageException.class).doCallRealMethod().when(logStorageFactory).destroyLogStorage(anyString()); + } + + @ParameterizedTest + @MethodSource("groupIdProvider") + void testFinishStorageDestructionAfterRestart(ReplicationGroupId replicationGroupId) throws Exception { + RaftNodeId nodeId = new RaftNodeId(replicationGroupId, peer); + + destroyStorageFails(server, nodeId, serverDataPath, false); + + Path nodeDataPath = createNodeDataDirectory(nodeId); + + restartServer(); + + assertFalse(Files.exists(nodeDataPath)); Review Comment: Let's assert that this directory exists before an attempt to destroy storages - just to make sure we are looking at the correct directory ########## modules/core/src/main/java/org/apache/ignite/internal/replicator/PartitionGroupId.java: ########## @@ -21,6 +21,9 @@ * A {@link ReplicationGroupId} which corresponds to partition of a partitioned object. */ public interface PartitionGroupId extends ReplicationGroupId { + /** Used for durable destruction purposes. */ + String PARTITION_GROUP_NAME = "partition"; Review Comment: I'm not sure that 'partition group ID' should know anything about destruction. Can we declare it elsewhere? ########## modules/raft/src/main/java/org/apache/ignite/internal/raft/server/impl/JraftServerImpl.java: ########## @@ -159,15 +166,21 @@ public class JraftServerImpl implements RaftServer { * @param opts Default node options. * @param raftGroupEventsClientListener Raft events listener. * @param failureManager Failure processor that is used to handle critical errors. + * @param groupStoragesDestructionIntents Storage to persist {@link DestroyStorageIntent}. Review Comment: ```suggestion * @param groupStoragesDestructionIntents Storage to persist {@link DestroyStorageIntent}s. ``` ########## modules/raft/src/main/java/org/apache/ignite/internal/raft/server/impl/JraftServerImpl.java: ########## @@ -573,16 +590,30 @@ 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. + DestroyStorageIntent intent = groupStoragesContextResolver.getIntent(nodeId, groupOptions.volatileStores()); + + groupStoragesDestructionIntents.saveDestroyStorageIntent(nodeId.groupId(), intent); + + destroyStorage(new DestroyStorageContext(intent, groupOptions.getLogStorageFactory(), groupOptions.serverDataPath())); + } + + private void destroyStorage( Review Comment: ```suggestion private void destroyStorages( ``` ########## modules/raft/src/main/java/org/apache/ignite/internal/raft/server/impl/JraftServerImpl.java: ########## @@ -573,16 +590,30 @@ 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. + DestroyStorageIntent intent = groupStoragesContextResolver.getIntent(nodeId, groupOptions.volatileStores()); + + groupStoragesDestructionIntents.saveDestroyStorageIntent(nodeId.groupId(), intent); + + destroyStorage(new DestroyStorageContext(intent, groupOptions.getLogStorageFactory(), groupOptions.serverDataPath())); + } + + private void destroyStorage( + DestroyStorageContext context + ) { + String nodeId = context.intent().nodeId(); + try { - String logUri = nodeId.nodeIdStringForStorage(); - groupOptions.getLogStorageFactory().destroyLogStorage(logUri); - } finally { - Path serverDataPath = serverDataPathForNodeId(nodeId, groupOptions); + if (context.logStorageFactory() != null) { + context.logStorageFactory().destroyLogStorage(nodeId); + } - // This destroys both meta storage and snapshots storage as they are stored under serverDataPath. - IgniteUtils.deleteIfExists(serverDataPath); + // This destroys both meta storage and snapshots storage as they are stored under nodeDataPath. + IgniteUtils.deleteIfExists(getServerDataPath(context.serverDataPath(), nodeId)); Review Comment: We might oversee an exception here. We probably need to do the following: 1. If the directory doesn't exist, we do nothing 2. Otherwise, we try to delete it 3. If an exception happens, we rethrow it ########## modules/raft/src/main/java/org/apache/ignite/internal/raft/storage/impl/VaultGroupStoragesDestructionIntents.java: ########## @@ -0,0 +1,129 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.internal.raft.storage.impl; + +import static java.nio.charset.StandardCharsets.UTF_8; +import static org.apache.ignite.lang.ErrorGroups.Common.INTERNAL_ERR; + +import java.io.IOException; +import java.nio.ByteBuffer; +import java.nio.ByteOrder; +import java.util.ArrayList; +import java.util.Collection; +import org.apache.ignite.internal.lang.ByteArray; +import org.apache.ignite.internal.lang.IgniteInternalException; +import org.apache.ignite.internal.raft.storage.GroupStoragesDestructionIntents; +import org.apache.ignite.internal.replicator.ReplicationGroupId; +import org.apache.ignite.internal.util.Cursor; +import org.apache.ignite.internal.util.io.IgniteUnsafeDataInput; +import org.apache.ignite.internal.util.io.IgniteUnsafeDataOutput; +import org.apache.ignite.internal.vault.VaultEntry; +import org.apache.ignite.internal.vault.VaultManager; + +/** Uses VaultManager to store destruction intents. */ +public class VaultGroupStoragesDestructionIntents implements GroupStoragesDestructionIntents { + /** Initial capacity (in bytes) of the buffer used for data output. */ + private static final int INITIAL_BUFFER_CAPACITY = 64; + + private static final byte[] GROUP_STORAGE_DESTRUCTION_PREFIX = "destroy.group.storages.".getBytes(UTF_8); + private static final ByteOrder BYTE_UTILS_BYTE_ORDER = ByteOrder.BIG_ENDIAN; + + private final VaultManager vault; + + /** Constructor. */ + public VaultGroupStoragesDestructionIntents(VaultManager vault) { + this.vault = vault; + } + + @Override + public void saveDestroyStorageIntent(ReplicationGroupId groupId, DestroyStorageIntent destroyStorageIntent) { + vault.put(buildKey(destroyStorageIntent.nodeId()), toStorageBytes(destroyStorageIntent)); + } + + private static byte[] toStorageBytes(DestroyStorageIntent intent) { + try (IgniteUnsafeDataOutput out = new IgniteUnsafeDataOutput(INITIAL_BUFFER_CAPACITY)) { Review Comment: Let's use a versioned serializer ########## modules/raft/src/main/java/org/apache/ignite/internal/raft/storage/impl/DestroyStorageIntent.java: ########## @@ -0,0 +1,52 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.internal.raft.storage.impl; + + +import org.apache.ignite.internal.tostring.S; + +/** Intent to destroy raft node's group storages. */ +public class DestroyStorageIntent { Review Comment: ```suggestion public class StoragesDestructionIntent { ``` ########## modules/raft/src/integrationTest/java/org/apache/ignite/raft/server/ItJraftDestructorTest.java: ########## @@ -0,0 +1,145 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.raft.server; + +import static org.apache.ignite.internal.testframework.IgniteTestUtils.assertThrows; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Objects; +import java.util.stream.Stream; +import org.apache.ignite.internal.cluster.management.CmgGroupId; +import org.apache.ignite.internal.lang.IgniteInternalException; +import org.apache.ignite.internal.metastorage.server.raft.MetastorageGroupId; +import org.apache.ignite.internal.raft.Peer; +import org.apache.ignite.internal.raft.RaftNodeId; +import org.apache.ignite.internal.raft.server.RaftGroupOptions; +import org.apache.ignite.internal.raft.server.impl.JraftServerImpl; +import org.apache.ignite.internal.raft.storage.LogStorageFactory; +import org.apache.ignite.internal.raft.storage.impl.LogStorageException; +import org.apache.ignite.internal.replicator.ReplicationGroupId; +import org.apache.ignite.internal.replicator.TablePartitionId; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.MethodSource; + +/** Tests that check that failed storage destruction is finished after server restart. */ +public class ItJraftDestructorTest extends JraftAbstractTest { + + private static Stream<ReplicationGroupId> groupIdProvider() { + return Stream.of( + new TablePartitionId(0, 0), + CmgGroupId.INSTANCE, + MetastorageGroupId.INSTANCE + ); + } + + private JraftServerImpl server; + private Path serverDataPath; + private Peer peer; + + @BeforeEach + void setUp() { + server = startServer(); + + serverDataPath = serverWorkingDirs.get(0).basePath(); + + String localNodeName = server.clusterService().topologyService().localMember().name(); + + peer = Objects.requireNonNull(initialMembersConf.peer(localNodeName)); + + LogStorageFactory logStorageFactory = logStorageFactories.get(0); + + doThrow(LogStorageException.class).doCallRealMethod().when(logStorageFactory).destroyLogStorage(anyString()); + } + + @ParameterizedTest + @MethodSource("groupIdProvider") + void testFinishStorageDestructionAfterRestart(ReplicationGroupId replicationGroupId) throws Exception { + RaftNodeId nodeId = new RaftNodeId(replicationGroupId, peer); + + destroyStorageFails(server, nodeId, serverDataPath, false); + + Path nodeDataPath = createNodeDataDirectory(nodeId); + + restartServer(); + + assertFalse(Files.exists(nodeDataPath)); + + // New log storage factory was created after restart. + verify(logStorageFactories.get(0), times(1)).destroyLogStorage(anyString()); + } + + @Test + void testVolatileLogsDontGetDestroyedOnRestart() throws Exception { + RaftNodeId nodeId = new RaftNodeId(new TablePartitionId(0, 0), peer); + + destroyStorageFails(server, nodeId, serverDataPath, true); + + Path nodeDataPath = createNodeDataDirectory(nodeId); + + restartServer(); + + assertFalse(Files.exists(nodeDataPath)); + + // New log storage factory was created after restart. + verify(logStorageFactories.get(0), times(0)).destroyLogStorage(anyString()); + } + + private void destroyStorageFails( Review Comment: ```suggestion private void attemptDestroyStoragesAndExpectFailure( ``` ########## modules/raft/src/main/java/org/apache/ignite/internal/raft/server/impl/GroupStoragesContextResolver.java: ########## @@ -0,0 +1,56 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.internal.raft.server.impl; + +import java.nio.file.Path; +import java.util.Map; +import java.util.function.Function; +import org.apache.ignite.internal.raft.RaftNodeId; +import org.apache.ignite.internal.raft.storage.LogStorageFactory; +import org.apache.ignite.internal.raft.storage.impl.DestroyStorageContext; +import org.apache.ignite.internal.raft.storage.impl.DestroyStorageIntent; +import org.apache.ignite.internal.replicator.ReplicationGroupId; + +/** Resolves {@link LogStorageFactory} and server data path for given {@link DestroyStorageIntent}. */ +public class GroupStoragesContextResolver { + private final Function<ReplicationGroupId, String> groupNameResolver; + + private final Map<String, Path> serverDataPathByGroupName; + private final Map<String, LogStorageFactory> logStorageFactoryByGroupName; + + /** Constructor. */ + public GroupStoragesContextResolver( + Function<ReplicationGroupId, String> groupNameResolver, + Map<String, Path> serverDataPathByGroupName, + Map<String, LogStorageFactory> logStorageFactoryByGroupName + ) { + this.groupNameResolver = groupNameResolver; + this.serverDataPathByGroupName = serverDataPathByGroupName; + this.logStorageFactoryByGroupName = logStorageFactoryByGroupName; Review Comment: Let's make defensive copies ########## modules/distribution-zones/src/integrationTest/java/org/apache/ignite/internal/rebalance/ItRebalanceDistributedTest.java: ########## @@ -1236,7 +1239,9 @@ private class Node { raftConfiguration, hybridClock, raftGroupEventsClientListener, - new NoOpFailureManager() + new NoOpFailureManager(), + new NoopGroupStoragesDestructionIntents(), + new GroupStoragesContextResolver(Objects::toString, Map.of(), Map.of()) Review Comment: Would it make sense to hide this in a no-arg constructor (test-only) to avoid copying this over and over? ########## modules/raft/src/main/java/org/apache/ignite/internal/raft/storage/impl/DestroyStorageContext.java: ########## @@ -0,0 +1,58 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.internal.raft.storage.impl; + +import java.nio.file.Path; +import org.apache.ignite.internal.raft.storage.LogStorageFactory; +import org.jetbrains.annotations.Nullable; + +/** Contains {@link DestroyStorageIntent}, server data path and {@link LogStorageFactory} for storage destruction. */ +public class DestroyStorageContext { Review Comment: ```suggestion public class StoragesDestructionContext { ``` ########## modules/raft/src/main/java/org/apache/ignite/internal/raft/server/impl/JraftServerImpl.java: ########## @@ -159,15 +166,21 @@ public class JraftServerImpl implements RaftServer { * @param opts Default node options. * @param raftGroupEventsClientListener Raft events listener. * @param failureManager Failure processor that is used to handle critical errors. + * @param groupStoragesDestructionIntents Storage to persist {@link DestroyStorageIntent}. + * @param groupStoragesContextResolver Resolver to get {@link DestroyStorageContext} for storage destruction. Review Comment: ```suggestion * @param groupStoragesContextResolver Resolver to get {@link DestroyStorageContext}s for storage destruction. ``` ########## modules/raft/src/integrationTest/java/org/apache/ignite/raft/server/JraftAbstractTest.java: ########## @@ -201,7 +220,39 @@ protected JraftServerImpl startServer(int idx, Consumer<RaftServer> clo, Consume optionsUpdater.accept(opts); - JraftServerImpl server = TestJraftServerFactory.create(service, opts); + VaultManager vaultManager = new VaultManager(new PersistentVaultService(vaultPath(workingDir.basePath()))); + + vaultManagers.add(vaultManager); + + assertThat(vaultManager.startAsync(new ComponentContext()), willCompleteSuccessfully()); + + Map<String, LogStorageFactory> logStorageFactoryByGroupName = Map.of( + PARTITION_GROUP_NAME, partitionsLogStorageFactory, + CmgGroupId.INSTANCE.toString(), partitionsLogStorageFactory, + MetastorageGroupId.INSTANCE.toString(), partitionsLogStorageFactory + ); + + Map<String, Path> serverDataPathByGroupName = Map.of( + PARTITION_GROUP_NAME, workingDir.basePath(), + CmgGroupId.INSTANCE.toString(), workingDir.basePath(), + MetastorageGroupId.INSTANCE.toString(), workingDir.basePath() + ); Review Comment: Why do we need this 'partitions, metastorage, cmg' specifics in a test that is about Raft and nothing more? ########## modules/raft/src/main/java/org/apache/ignite/internal/raft/server/impl/JraftServerImpl.java: ########## @@ -573,16 +590,30 @@ 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. + DestroyStorageIntent intent = groupStoragesContextResolver.getIntent(nodeId, groupOptions.volatileStores()); + + groupStoragesDestructionIntents.saveDestroyStorageIntent(nodeId.groupId(), intent); + + destroyStorage(new DestroyStorageContext(intent, groupOptions.getLogStorageFactory(), groupOptions.serverDataPath())); + } + + private void destroyStorage( + DestroyStorageContext context Review Comment: Do we need a separate line just for 1 argument? ########## modules/runner/src/main/java/org/apache/ignite/internal/app/IgniteImpl.java: ########## @@ -1186,6 +1200,26 @@ public class IgniteImpl implements Ignite { publicCatalog = new PublicApiThreadingIgniteCatalog(new IgniteCatalogSqlImpl(sql, distributedTblMgr), asyncContinuationExecutor); } + private GroupStoragesContextResolver groupStoragesContextResolver() { Review Comment: ```suggestion private GroupStoragesContextResolver createGroupStoragesContextResolver() { ``` -- 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
