Skip to content

Commit

Permalink
Make alluxioOnly delete not sync with UFS
Browse files Browse the repository at this point in the history 
### What changes are proposed in this pull request?

When the deletion is alluxio-only, the sync is skipped.

### Why are the changes needed?

When the deletion is alluxio-only, there should be no need to sync with
the UFS. This should make large alluxio-only deletion requests much
faster.

### Does this PR introduce any user facing changes?

NA

pr-link: Alluxio#15009
change-id: cid-28ed020bf94dc356c2374b8afc9c22748f917ec6
  • Loading branch information
@jiacheliu3
jiacheliu3 authored Feb 23, 2022
1 parent c9f1b1c commit 2f0c990
Show file tree
Hide file tree
Showing 3 changed files with 108 additions and 13 deletions.
28 changes: 17 additions & 11 deletions core/server/master/src/main/java/alluxio/master/file/DefaultFileSystemMaster.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@
* The master that handles all file system metadata management.
*/
@NotThreadSafe // TODO(jiri): make thread-safe (c.f. ALLUXIO-1664)
public final class DefaultFileSystemMaster extends CoreMaster
public class DefaultFileSystemMaster extends CoreMaster
implements FileSystemMaster, DelegatingJournaled, Reconfigurable {
private static final Logger LOG = LoggerFactory.getLogger(DefaultFileSystemMaster.class);
private static final Set<Class<? extends Server>> DEPS = ImmutableSet.of(BlockMaster.class);
Expand Down Expand Up @@ -1822,15 +1822,20 @@ public void delete(AlluxioURI path, DeleteContext context)
FileSystemMasterAuditContext auditContext =
createAuditContext("delete", path, null, null)) {

syncMetadata(rpcContext,
path,
context.getOptions().getCommonOptions(),
context.getOptions().getRecursive() ? DescendantType.ALL : DescendantType.ONE,
auditContext,
LockedInodePath::getInodeOrNull,
(inodePath, permChecker) -> permChecker.checkParentPermission(Mode.Bits.WRITE, inodePath),
false
);
if (context.getOptions().getAlluxioOnly()) {
LOG.debug("alluxio-only deletion on path {} skips metadata sync", path);
} else {
syncMetadata(rpcContext,
path,
context.getOptions().getCommonOptions(),
context.getOptions().getRecursive() ? DescendantType.ALL : DescendantType.ONE,
auditContext,
LockedInodePath::getInodeOrNull,
(inodePath, permChecker) ->
permChecker.checkParentPermission(Mode.Bits.WRITE, inodePath),
false
);
}

LockingScheme lockingScheme =
createLockingScheme(path, context.getOptions().getCommonOptions(),
Expand Down Expand Up @@ -3564,7 +3569,8 @@ public boolean recordActiveSyncTxid(long txId, long mountId) {
* @param isGetFileInfo true if syncing for a getFileInfo operation
* @return syncStatus
*/
private InodeSyncStream.SyncStatus syncMetadata(RpcContext rpcContext, AlluxioURI path,
@VisibleForTesting
InodeSyncStream.SyncStatus syncMetadata(RpcContext rpcContext, AlluxioURI path,
FileSystemMasterCommonPOptions options, DescendantType syncDescendantType,
@Nullable FileSystemMasterAuditContext auditContext,
@Nullable Function<LockedInodePath, Inode> auditContextSrcInodeFunc,
Expand Down
91 changes: 90 additions & 1 deletion core/server/master/src/test/java/alluxio/master/file/FileSystemMasterSyncMetadataTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
package alluxio.master.file;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotEquals;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.ArgumentMatchers.eq;
Expand All @@ -23,19 +25,26 @@
import alluxio.exception.FileAlreadyExistsException;
import alluxio.exception.FileDoesNotExistException;
import alluxio.exception.InvalidPathException;
import alluxio.file.options.DescendantType;
import alluxio.grpc.DeletePOptions;
import alluxio.grpc.FileSystemMasterCommonPOptions;
import alluxio.grpc.GetStatusPOptions;
import alluxio.grpc.ListStatusPOptions;
import alluxio.heartbeat.HeartbeatContext;
import alluxio.heartbeat.ManuallyScheduleHeartbeat;
import alluxio.master.CoreMasterContext;
import alluxio.master.MasterFactory;
import alluxio.master.MasterRegistry;
import alluxio.master.MasterTestUtils;
import alluxio.master.block.BlockMaster;
import alluxio.master.block.BlockMasterFactory;
import alluxio.master.file.contexts.CreateDirectoryContext;
import alluxio.master.file.contexts.DeleteContext;
import alluxio.master.file.contexts.GetStatusContext;
import alluxio.master.file.contexts.ListStatusContext;
import alluxio.master.file.contexts.MountContext;
import alluxio.master.file.meta.Inode;
import alluxio.master.file.meta.LockedInodePath;
import alluxio.master.journal.JournalSystem;
import alluxio.master.journal.JournalTestUtils;
import alluxio.master.journal.JournalType;
Expand All @@ -47,7 +56,11 @@
import alluxio.underfs.UfsFileStatus;
import alluxio.underfs.UfsStatus;
import alluxio.underfs.UnderFileSystem;
import alluxio.util.IdUtils;
import alluxio.util.ModeUtils;
import alluxio.util.ThreadFactoryUtils;
import alluxio.util.executor.ExecutorServiceFactories;
import alluxio.util.executor.ExecutorServiceFactory;
import alluxio.util.io.PathUtils;
import alluxio.wire.FileInfo;

Expand All @@ -62,9 +75,14 @@
import org.powermock.core.classloader.annotations.PrepareForTest;
import org.powermock.modules.junit4.PowerMockRunner;

import javax.annotation.Nullable;
import java.io.File;
import java.io.IOException;
import java.util.List;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.function.Function;

/**
* Unit tests for {@link FileSystemMaster}.
Expand All @@ -76,6 +94,7 @@ public final class FileSystemMasterSyncMetadataTest {
private MasterRegistry mRegistry;
private FileSystemMaster mFileSystemMaster;
private UnderFileSystem mUfs;
private ExecutorService mExecutorService;

@Rule
public ManuallyScheduleHeartbeat mManualScheduler =
Expand Down Expand Up @@ -225,14 +244,84 @@ private AlluxioURI setupMockUfsS3Mount()
return ufsMount;
}

@Test
public void deleteAlluxioOnlyNoSync() throws Exception {
// Prepare files
mFileSystemMaster.createDirectory(new AlluxioURI("/a/"), CreateDirectoryContext.defaults());
mFileSystemMaster.createDirectory(new AlluxioURI("/a/b/"), CreateDirectoryContext.defaults());
mFileSystemMaster.createDirectory(new AlluxioURI("/b/"), CreateDirectoryContext.defaults());
// If the sync operation happens, the flag will be marked
SyncAwareFileSystemMaster delegateMaster = (SyncAwareFileSystemMaster) mFileSystemMaster;
delegateMaster.setSynced(false);

delegateMaster.delete(new AlluxioURI("/a/"),
DeleteContext.mergeFrom(DeletePOptions.newBuilder()
.setRecursive(true).setAlluxioOnly(true)));
// The files have been deleted
assertEquals(IdUtils.INVALID_FILE_ID, mFileSystemMaster.getFileId(new AlluxioURI("/a/")));
assertEquals(IdUtils.INVALID_FILE_ID, mFileSystemMaster.getFileId(new AlluxioURI("/a/b/")));
// Irrelevant files are not affected
assertNotEquals(IdUtils.INVALID_FILE_ID, mFileSystemMaster.getFileId(new AlluxioURI("/b/")));
// Sync has not happened
assertFalse(delegateMaster.mSynced.get());
}

private static class SyncAwareFileSystemMaster extends DefaultFileSystemMaster {
AtomicBoolean mSynced = new AtomicBoolean(false);

public SyncAwareFileSystemMaster(BlockMaster blockMaster, CoreMasterContext masterContext,
ExecutorServiceFactory executorServiceFactory) {
super(blockMaster, masterContext, executorServiceFactory);
}

@Override
InodeSyncStream.SyncStatus syncMetadata(RpcContext rpcContext, AlluxioURI path,
FileSystemMasterCommonPOptions options, DescendantType syncDescendantType,
@Nullable FileSystemMasterAuditContext auditContext,
@Nullable Function<LockedInodePath, Inode> auditContextSrcInodeFunc,
@Nullable PermissionCheckFunction permissionCheckOperation,
boolean isGetFileInfo) throws AccessControlException, InvalidPathException {
mSynced.set(true);
return super.syncMetadata(rpcContext, path, options, syncDescendantType, auditContext,
auditContextSrcInodeFunc, permissionCheckOperation, isGetFileInfo);
}

void setSynced(boolean synced) {
mSynced.set(synced);
}
}

private class SyncAwareFileSystemMasterFactory implements MasterFactory<CoreMasterContext> {
@Override
public boolean isEnabled() {
return true;
}

@Override
public String getName() {
return "SyncAwareFileSystemMasterFactory";
}

@Override
public FileSystemMaster create(MasterRegistry registry, CoreMasterContext context) {
BlockMaster blockMaster = registry.get(BlockMaster.class);
FileSystemMaster fileSystemMaster = new SyncAwareFileSystemMaster(blockMaster, context,
ExecutorServiceFactories.constantExecutorServiceFactory(mExecutorService));
registry.add(FileSystemMaster.class, fileSystemMaster);
return fileSystemMaster;
}
}

private void startServices() throws Exception {
mExecutorService = Executors
.newFixedThreadPool(4, ThreadFactoryUtils.build("DefaultFileSystemMasterTest-%d", true));
mRegistry = new MasterRegistry();
JournalSystem journalSystem =
JournalTestUtils.createJournalSystem(mJournalFolder.getAbsolutePath());
CoreMasterContext context = MasterTestUtils.testMasterContext(journalSystem);
new MetricsMasterFactory().create(mRegistry, context);
new BlockMasterFactory().create(mRegistry, context);
mFileSystemMaster = new FileSystemMasterFactory().create(mRegistry, context);
mFileSystemMaster = new SyncAwareFileSystemMasterFactory().create(mRegistry, context);
journalSystem.start();
journalSystem.gainPrimacy();
mRegistry.start(true);
Expand Down
2 changes: 1 addition & 1 deletion shell/src/main/java/alluxio/cli/fs/command/RmCommand.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ protected void runPlainPath(AlluxioURI path, CommandLine cl)
if (!isAlluxioOnly) {
System.out.println(path + " has been removed");
} else {
System.out.println(path + " has been removed from Alluxio space");
System.out.println(path + " has been removed only from Alluxio space");
}
}

Expand Down

0 comments on commit 2f0c990

Please sign in to comment.