You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ratis.apache.org by GitBox <gi...@apache.org> on 2020/07/30 10:50:03 UTC
[GitHub] [incubator-ratis] lokeshj1703 commented on a change in pull request #148: RATIS-924: rename raft group dir on disk when remove group is invoked
lokeshj1703 commented on a change in pull request #148:
URL: https://github.com/apache/incubator-ratis/pull/148#discussion_r462898041
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/RaftServerConfigKeys.java
##########
@@ -50,9 +50,21 @@ static void setStorageDir(RaftProperties properties, List<File> storageDir) {
setFiles(properties::setFiles, STORAGE_DIR_KEY, storageDir);
}
- String REMOVED_GROUPS_DIR_KEY = STORAGE_DIR_KEY + ".removed.groups.dir";
+ String MOVE_REMOVED_GROUPS_ENABLED_KEY = PREFIX
+ + ".move.removed.groups.enabled";
+ boolean MOVE_REMOVED_GROUPS_ENABLED_DEFAULT = false;
+ static boolean moveRemovedGroupsEnabled(RaftProperties properties) {
+ return getBoolean(properties::getBoolean, MOVE_REMOVED_GROUPS_ENABLED_KEY,
+ MOVE_REMOVED_GROUPS_ENABLED_DEFAULT, getDefaultLog());
+ }
+ static void setMoveRemovedGroupsEnabled(RaftProperties properties,
+ boolean shouldMoveRemovedGroups) {
+ setBoolean(properties::setBoolean, MOVE_REMOVED_GROUPS_ENABLED_KEY,
+ shouldMoveRemovedGroups);
+ }
Review comment:
I think we should add a field for renameDirectory in GroupRemoveRequestProto. This would be similar to deleteDirectory field. Then we do not need this configuration.
##########
File path: ratis-common/src/main/java/org/apache/ratis/util/FileUtils.java
##########
@@ -85,6 +85,43 @@ static void deleteFile(File f) throws IOException {
delete(f.toPath());
}
+ /**
+ * Moves the directory. If any file is locked, the exception is caught
+ * and logged and continues to other files.
+ * @param source
+ * @param dest
+ * @throws IOException
+ */
+ static void moveDirectory(Path source, Path dest) throws IOException {
+ LOG.trace("moveDirectory source: {} dest: {}", source, dest);
Review comment:
```suggestion
if (LOG.isTraceEnabled()) {
LOG.trace("moveDirectory source: {} dest: {}", source, dest);
}
```
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java
##########
@@ -258,6 +259,39 @@ RaftGroup getGroup() {
return RaftGroup.valueOf(getMemberId().getGroupId(), getRaftConf().getPeers());
}
+ /**
+ * This removes the group from the server.
+ * If the flag is set to false, the directory is moved to
+ * {@link RaftServerConfigKeys#REMOVED_GROUPS_DIR_KEY} location only if
+ * {@link RaftServerConfigKeys#MOVE_REMOVED_GROUPS_ENABLED_KEY} is enabled.
+ * If the flag is true, the group is permanently deleted.
+ * @param deleteDirectory
+ */
+ public void groupRemove(boolean deleteDirectory) {
+ final RaftStorageDirectory dir = state.getStorage().getStorageDir();
+
+ /* Shutdown is triggered here inorder to avoid any locked files. */
+ shutdown(deleteDirectory);
+
+ if(!deleteDirectory && RaftServerConfigKeys.moveRemovedGroupsEnabled
Review comment:
I think we can move the logic of delete directory from shutdown to this function.
##########
File path: ratis-common/src/main/java/org/apache/ratis/util/FileUtils.java
##########
@@ -85,6 +85,43 @@ static void deleteFile(File f) throws IOException {
delete(f.toPath());
}
+ /**
+ * Moves the directory. If any file is locked, the exception is caught
+ * and logged and continues to other files.
+ * @param source
+ * @param dest
+ * @throws IOException
+ */
+ static void moveDirectory(Path source, Path dest) throws IOException {
+ LOG.trace("moveDirectory source: {} dest: {}", source, dest);
+ createDirectories(dest);
+ Files.walkFileTree(source, new SimpleFileVisitor<Path>() {
+ @Override
+ public FileVisitResult preVisitDirectory(Path dir,
+ BasicFileAttributes attrs) throws IOException {
+ Path targetPath = dest.resolve(source.relativize(dir));
+ if (!Files.exists(targetPath)) {
+ createDirectories(targetPath);
+ }
Review comment:
Since we are making doing createDirectories call for dest, I do not think we need this.
##########
File path: ratis-server/src/test/java/org/apache/ratis/server/impl/GroupManagementBaseTest.java
##########
@@ -255,4 +258,49 @@ public void testGroupAlreadyExists() throws Exception {
cluster.shutdown();
}
}
+
+ @Test
+ public void testGroupRemove() throws Exception {
Review comment:
I see an exception while running the test.
```
2020-07-30 16:00:49,922 [grpc-default-executor-0] TRACE util.FileUtils (FileUtils.java:moveDirectory(97)) - moveDirectory source: /Users/ljain/codebase/apache/incubator-ratis-2/ratis-test/target/test/data/e87f4236/MiniRaftClusterWithGrpc77d65499/s0/ee7ea209-68e0-4fbd-97ce-f85fd0542aee dest: /private/var/folders/2j/h06ldsm970b_jyqskbqwz49m0000gn/T/groups5576348764218221965/ee7ea209-68e0-4fbd-97ce-f85fd0542aee
2020-07-30 16:00:49,922 [grpc-default-executor-0] TRACE util.FileUtils (LogUtils.java:runAndLog(49)) - Successfully ran Files.createDirectories /private/var/folders/2j/h06ldsm970b_jyqskbqwz49m0000gn/T/groups5576348764218221965/ee7ea209-68e0-4fbd-97ce-f85fd0542aee
2020-07-30 16:00:49,926 [grpc-default-executor-0] TRACE util.FileUtils (LogUtils.java:runAndLog(41)) - Failed to Files.move /Users/ljain/codebase/apache/incubator-ratis-2/ratis-test/target/test/data/e87f4236/MiniRaftClusterWithGrpc77d65499/s0/ee7ea209-68e0-4fbd-97ce-f85fd0542aee/current/raft-meta to /private/var/folders/2j/h06ldsm970b_jyqskbqwz49m0000gn/T/groups5576348764218221965/ee7ea209-68e0-4fbd-97ce-f85fd0542aee/current/raft-meta
java.nio.file.NoSuchFileException: /Users/ljain/codebase/apache/incubator-ratis-2/ratis-test/target/test/data/e87f4236/MiniRaftClusterWithGrpc77d65499/s0/ee7ea209-68e0-4fbd-97ce-f85fd0542aee/current/raft-meta -> /private/var/folders/2j/h06ldsm970b_jyqskbqwz49m0000gn/T/groups5576348764218221965/ee7ea209-68e0-4fbd-97ce-f85fd0542aee/current/raft-meta
at sun.nio.fs.UnixException.translateToIOException(UnixException.java:86)
at sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:102)
at sun.nio.fs.UnixCopyFile.move(UnixCopyFile.java:457)
at sun.nio.fs.UnixFileSystemProvider.move(UnixFileSystemProvider.java:262)
at java.nio.file.Files.move(Files.java:1395)
at org.apache.ratis.util.FileUtils.lambda$move$6(FileUtils.java:79)
at org.apache.ratis.util.LogUtils.runAndLog(LogUtils.java:38)
at org.apache.ratis.util.FileUtils.move(FileUtils.java:78)
at org.apache.ratis.util.FileUtils$1.visitFile(FileUtils.java:114)
at org.apache.ratis.util.FileUtils$1.visitFile(FileUtils.java:100)
at java.nio.file.Files.walkFileTree(Files.java:2670)
at java.nio.file.Files.walkFileTree(Files.java:2742)
at org.apache.ratis.util.FileUtils.moveDirectory(FileUtils.java:100)
at org.apache.ratis.server.impl.RaftServerImpl.groupRemove(RaftServerImpl.java:284)
at org.apache.ratis.server.impl.RaftServerProxy.lambda$groupRemoveAsync$12(RaftServerProxy.java:418)
at java.util.concurrent.CompletableFuture.uniApply(CompletableFuture.java:602)
at java.util.concurrent.CompletableFuture.uniApplyStage(CompletableFuture.java:614)
at java.util.concurrent.CompletableFuture.thenApply(CompletableFuture.java:1983)
at org.apache.ratis.server.impl.RaftServerProxy.groupRemoveAsync(RaftServerProxy.java:416)
at org.apache.ratis.server.impl.RaftServerProxy.groupManagementAsync(RaftServerProxy.java:372)
at org.apache.ratis.grpc.server.GrpcAdminProtocolService.lambda$groupManagement$0(GrpcAdminProtocolService.java:43)
at org.apache.ratis.grpc.GrpcUtil.asyncCall(GrpcUtil.java:165)
at org.apache.ratis.grpc.server.GrpcAdminProtocolService.groupManagement(GrpcAdminProtocolService.java:43)
at org.apache.ratis.proto.grpc.AdminProtocolServiceGrpc$MethodHandlers.invoke(AdminProtocolServiceGrpc.java:367)
at org.apache.ratis.thirdparty.io.grpc.stub.ServerCalls$UnaryServerCallHandler$UnaryServerCallListener.onHalfClose(ServerCalls.java:172)
at org.apache.ratis.thirdparty.io.grpc.internal.ServerCallImpl$ServerStreamListenerImpl.halfClosed(ServerCallImpl.java:331)
at org.apache.ratis.thirdparty.io.grpc.internal.ServerImpl$JumpToApplicationThreadServerStreamListener$1HalfClosed.runInContext(ServerImpl.java:818)
at org.apache.ratis.thirdparty.io.grpc.internal.ContextRunnable.run(ContextRunnable.java:37)
at org.apache.ratis.thirdparty.io.grpc.internal.SerializingExecutor.run(SerializingExecutor.java:123)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
at java.lang.Thread.run(Thread.java:748)
2020-07-30 16:00:49,928 [grpc-default-executor-0] INFO util.FileUtils (FileUtils.java:visitFile(116)) - Files.moveDirectory: could not delete raft-meta
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org