Fix concurrent sync dedup

Cherry-pick of existing commit. orig-pr: Alluxio#16717 orig-commit: Alluxio/alluxio@12ecbc8 orig-commit-author: elega <445092967@qq.com> pr-link: Alluxio#17121 change-id: cid-d1ab7d5aa81905bcb2d394b5e922a33cf747908b
Xenorith · Mar 20, 2023 · 5307320 · 5307320
1 parent 0393275
commit 5307320
Show file tree

Hide file tree

Showing 3 changed files with 28 additions and 3 deletions.
diff --git a/core/server/master/src/main/java/alluxio/master/file/InodeSyncStream.java b/core/server/master/src/main/java/alluxio/master/file/InodeSyncStream.java
@@ -48,6 +48,7 @@
 import alluxio.master.file.meta.LockingScheme;
 import alluxio.master.file.meta.MountTable;
 import alluxio.master.file.meta.MutableInodeFile;
+import alluxio.master.file.meta.SyncCheck;
 import alluxio.master.file.meta.SyncCheck.SyncResult;
 import alluxio.master.file.meta.UfsAbsentPathCache;
 import alluxio.master.file.meta.UfsSyncPathCache;
@@ -435,7 +436,7 @@ private SyncStatus syncInternal() throws
       DefaultFileSystemMaster.Metrics.INODE_SYNC_STREAM_SKIPPED.inc();
       return SyncStatus.NOT_NEEDED;
     }
-    if (mDedupConcurrentSync) {
+    if (mDedupConcurrentSync && mRootScheme.shouldSync() != SyncCheck.SHOULD_SYNC) {
       /*
        * If a concurrent sync on the same path is successful after this sync had already
        * been initialized and that sync is successful, then there is no need to sync again.
@@ -456,9 +457,10 @@ private SyncStatus syncInternal() throws
        * Note that this still applies if A is to sync recursively path /aaa while B is to
        * sync path /aaa/bbb as the sync scope of A covers B's.
        */
-      boolean shouldSync = mUfsSyncPathCache.shouldSyncPath(mRootScheme.getPath(), mSyncInterval,
+      boolean shouldSkipSync =
+          mUfsSyncPathCache.shouldSyncPath(mRootScheme.getPath(), mSyncInterval,
           mDescendantType).getLastSyncTime() > mRootScheme.shouldSync().getLastSyncTime();
-      if (shouldSync) {
+      if (shouldSkipSync) {
         DefaultFileSystemMaster.Metrics.INODE_SYNC_STREAM_SKIPPED.inc();
         LOG.debug("Skipped sync on {} due to successful concurrent sync", mRootScheme.getPath());
         return SyncStatus.NOT_NEEDED;

diff --git a/core/server/master/src/main/java/alluxio/master/file/meta/LockingScheme.java b/core/server/master/src/main/java/alluxio/master/file/meta/LockingScheme.java
@@ -32,13 +32,19 @@ public final class LockingScheme {
   private final LockPattern mDesiredLockPattern;
   private final SyncCheck mShouldSync;
 
+  // CHECKSTYLE.OFF: LineLengthExceed - cannot break the method link
   /**
    * Constructs a {@link LockingScheme}.
    *
+   * Avoid using this constructor where shouldSync is set true, if possible.
+   * {@link #LockingScheme(AlluxioURI, LockPattern, FileSystemMasterCommonPOptions, UfsSyncPathCache, DescendantType)}
+   * is the preferred one in such case, to make the metadata sync dedup feature work.
+   *
    * @param path the path to lock
    * @param desiredLockPattern the desired lock mode
    * @param shouldSync true if the path should be synced
    */
+  // CHECKSTYLE.ON: LineLengthExceed
   public LockingScheme(AlluxioURI path, LockPattern desiredLockPattern, boolean shouldSync) {
     mPath = path;
     mDesiredLockPattern = desiredLockPattern;

diff --git a/.../master/src/test/java/alluxio/master/file/FileSystemMasterSyncMetadataConcurrentTest.java b/.../master/src/test/java/alluxio/master/file/FileSystemMasterSyncMetadataConcurrentTest.java
@@ -162,6 +162,23 @@ public void syncTheSameDirectoryButTheSecondCallCancelled() throws Exception {
     assertEquals(InodeSyncStream.SyncStatus.OK, iss3.sync());
   }
 
+  @Test
+  public void syncWhenShouldSyncIsSetTrue() throws Exception {
+    Supplier<InodeSyncStream> inodeSyncStreamSupplier =  () -> new InodeSyncStream(
+        new LockingScheme(
+            new AlluxioURI("/"), InodeTree.LockPattern.READ, true),
+        mFileSystemMaster, mFileSystemMaster.getSyncPathCache(),
+        RpcContext.NOOP, DescendantType.ALL, FileSystemMasterCommonPOptions.getDefaultInstance(),
+        false,
+        false,
+        false);
+
+    InodeSyncStream iss1 = inodeSyncStreamSupplier.get();
+    InodeSyncStream iss2 = inodeSyncStreamSupplier.get();
+    assertSyncHappenTwice(syncConcurrent(iss1, iss2));
+    assertSyncHappenTwice(syncSequential(inodeSyncStreamSupplier, inodeSyncStreamSupplier));
+  }
+
   private void assertTheSecondSyncSkipped(
       Pair<InodeSyncStream.SyncStatus, InodeSyncStream.SyncStatus> results) {
     assertEquals(InodeSyncStream.SyncStatus.OK, results.getFirst());