GEODE-5398: Add BackupOperationTest and BackupStatusImplTest

Refactor BackupOperation to make it easier to unit test.
shjgiser · Jul 9, 2018 · 1eb0275 · 1eb0275
1 parent bb79fdb
commit 1eb0275
Show file tree

Hide file tree

Showing 15 changed files with 327 additions and 62 deletions.
diff --git a/geode-core/src/main/java/org/apache/geode/admin/internal/AdminDistributedSystemImpl.java b/geode-core/src/main/java/org/apache/geode/admin/internal/AdminDistributedSystemImpl.java
@@ -2322,7 +2322,8 @@ public BackupStatus backupAllMembers(File targetDir, File baselineDir) throws Ad
   public static BackupStatus backupAllMembers(DistributionManager dm, File targetDir,
       File baselineDir) throws AdminException {
     String baselineDirectory = baselineDir == null ? null : baselineDir.toString();
-    return new BackupOperation().backupAllMembers(dm, targetDir.toString(), baselineDirectory);
+    return new BackupOperation(dm, dm.getCache()).backupAllMembers(targetDir.toString(),
+        baselineDirectory);
   }
 
   public Map<DistributedMember, Set<PersistentID>> compactAllDiskStores() throws AdminException {

diff --git a/geode-core/src/main/java/org/apache/geode/internal/SystemAdmin.java b/geode-core/src/main/java/org/apache/geode/internal/SystemAdmin.java
@@ -619,7 +619,8 @@ public static void backup(String targetDir) throws AdminException {
 
     // Baseline directory should be null if it was not provided on the command line
     BackupStatus status =
-        new BackupOperation().backupAllMembers(ads.getDistributionManager(), targetDir,
+        new BackupOperation(ads.getDistributionManager(), ads.getCache()).backupAllMembers(
+            targetDir,
             SystemAdmin.baselineDir);
 
     boolean incomplete = !status.getOfflineDiskStores().isEmpty();

diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/backup/AbortBackupFactory.java b/geode-core/src/main/java/org/apache/geode/internal/cache/backup/AbortBackupFactory.java
@@ -42,4 +42,10 @@ BackupResponse createBackupResponse(InternalDistributedMember sender,
       HashSet<PersistentID> persistentIds) {
     return new BackupResponse(sender, persistentIds);
   }
+
+  AbortBackupStep createAbortBackupStep(DistributionManager dm, InternalDistributedMember member,
+      InternalCache cache, Set<InternalDistributedMember> recipients,
+      AbortBackupFactory abortBackupFactory) {
+    return new AbortBackupStep(dm, member, cache, recipients, abortBackupFactory);
+  }
 }
diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/backup/BackupOperation.java b/geode-core/src/main/java/org/apache/geode/internal/cache/backup/BackupOperation.java
@@ -20,99 +20,129 @@
 import java.util.Set;
 
 import org.apache.geode.admin.internal.AdminDistributedSystemImpl;
+import org.apache.geode.annotations.TestingOnly;
 import org.apache.geode.cache.persistence.PersistentID;
 import org.apache.geode.distributed.DistributedMember;
 import org.apache.geode.distributed.internal.DistributionManager;
 import org.apache.geode.distributed.internal.membership.InternalDistributedMember;
+import org.apache.geode.internal.cache.InternalCache;
 import org.apache.geode.internal.i18n.LocalizedStrings;
 import org.apache.geode.management.BackupStatus;
 import org.apache.geode.management.ManagementException;
 import org.apache.geode.management.internal.BackupStatusImpl;
 
+/**
+ * Performs backup of all members.
+ */
 public class BackupOperation {
 
   private final FlushToDiskFactory flushToDiskFactory;
   private final PrepareBackupFactory prepareBackupFactory;
   private final AbortBackupFactory abortBackupFactory;
   private final FinishBackupFactory finishBackupFactory;
+  private final DistributionManager dm;
+  private final InternalCache cache;
+  private final BackupLockService backupLockService;
+  private final MissingPersistentMembersProvider missingPersistentMembersProvider;
 
-  public BackupOperation() {
+  public BackupOperation(DistributionManager dm, InternalCache cache) {
     this(new FlushToDiskFactory(), new PrepareBackupFactory(), new AbortBackupFactory(),
-        new FinishBackupFactory());
+        new FinishBackupFactory(), dm, cache, new BackupLockService(),
+        new DefaultMissingPersistentMembersProvider());
   }
 
+  @TestingOnly
   BackupOperation(FlushToDiskFactory flushToDiskFactory, PrepareBackupFactory prepareBackupFactory,
-      AbortBackupFactory abortBackupFactory, FinishBackupFactory finishBackupFactory) {
+      AbortBackupFactory abortBackupFactory, FinishBackupFactory finishBackupFactory,
+      DistributionManager dm, InternalCache cache, BackupLockService backupLockService,
+      MissingPersistentMembersProvider missingPersistentMembersProvider) {
     this.flushToDiskFactory = flushToDiskFactory;
     this.prepareBackupFactory = prepareBackupFactory;
     this.abortBackupFactory = abortBackupFactory;
     this.finishBackupFactory = finishBackupFactory;
+    this.dm = dm;
+    this.cache = cache;
+    this.backupLockService = backupLockService;
+    this.missingPersistentMembersProvider = missingPersistentMembersProvider;
   }
 
-  public BackupStatus backupAllMembers(DistributionManager dm, String targetDirPath,
-      String baselineDirPath) {
+  public BackupStatus backupAllMembers(String targetDirPath, String baselineDirPath) {
     Properties properties = new BackupConfigFactory().withTargetDirPath(targetDirPath)
         .withBaselineDirPath(baselineDirPath).createBackupProperties();
-    return performBackup(dm, properties);
+    return performBackup(properties);
   }
 
-  private BackupStatus performBackup(DistributionManager dm, Properties properties)
-      throws ManagementException {
-    BackupLockService backupLockService = new BackupLockService();
-    BackupStatus status;
+  private BackupStatus performBackup(Properties properties) throws ManagementException {
     if (backupLockService.obtainLock(dm)) {
       try {
-        Set<PersistentID> missingMembers =
-            AdminDistributedSystemImpl.getMissingPersistentMembers(dm);
-        Set<InternalDistributedMember> recipients = dm.getOtherDistributionManagerIds();
-
-        BackupDataStoreResult result = performBackupSteps(dm, recipients, properties);
-
-        // It's possible that when calling getMissingPersistentMembers, some members are
-        // still creating/recovering regions, and at FinishBackupRequest.send, the
-        // regions at the members are ready. Logically, since the members in successfulMembers
-        // should override the previous missingMembers
-        for (Set<PersistentID> onlineMembersIds : result.getSuccessfulMembers().values()) {
-          missingMembers.removeAll(onlineMembersIds);
-        }
-
-        result.getExistingDataStores().keySet().removeAll(result.getSuccessfulMembers().keySet());
-        for (Set<PersistentID> lostMembersIds : result.getExistingDataStores().values()) {
-          missingMembers.addAll(lostMembersIds);
-        }
-        status = new BackupStatusImpl(result.getSuccessfulMembers(), missingMembers);
+        return performBackupUnderLock(properties);
       } finally {
         backupLockService.releaseLock(dm);
       }
-
     } else {
       throw new ManagementException(
           LocalizedStrings.DistributedSystem_BACKUP_ALREADY_IN_PROGRESS.toLocalizedString());
     }
-    return status;
   }
 
-  private BackupDataStoreResult performBackupSteps(DistributionManager dm, Set recipients,
-      Properties properties) {
-    new FlushToDiskStep(dm, dm.getId(), dm.getCache(), recipients, flushToDiskFactory).send();
+  private BackupStatus performBackupUnderLock(Properties properties) {
+    Set<PersistentID> missingMembers =
+        missingPersistentMembersProvider.getMissingPersistentMembers(dm);
+    Set<InternalDistributedMember> recipients = dm.getOtherDistributionManagerIds();
+
+    BackupDataStoreResult result = performBackupSteps(dm.getId(), recipients, properties);
+
+    // It's possible that when calling getMissingPersistentMembers, some members are
+    // still creating/recovering regions, and at FinishBackupRequest.send, the
+    // regions at the members are ready. Logically, since the members in successfulMembers
+    // should override the previous missingMembers
+    for (Set<PersistentID> onlineMembersIds : result.getSuccessfulMembers().values()) {
+      missingMembers.removeAll(onlineMembersIds);
+    }
+
+    result.getExistingDataStores().keySet().removeAll(result.getSuccessfulMembers().keySet());
+    for (Set<PersistentID> lostMembersIds : result.getExistingDataStores().values()) {
+      missingMembers.addAll(lostMembersIds);
+    }
+    return new BackupStatusImpl(result.getSuccessfulMembers(), missingMembers);
+  }
+
+  private BackupDataStoreResult performBackupSteps(InternalDistributedMember member,
+      Set<InternalDistributedMember> recipients, Properties properties) {
+    flushToDiskFactory.createFlushToDiskStep(dm, member, cache, recipients, flushToDiskFactory)
+        .send();
 
     boolean abort = true;
     Map<DistributedMember, Set<PersistentID>> successfulMembers;
     Map<DistributedMember, Set<PersistentID>> existingDataStores;
     try {
-      existingDataStores = new PrepareBackupStep(dm, dm.getId(), dm.getCache(), recipients,
-          prepareBackupFactory, properties).send();
+      PrepareBackupStep prepareBackupStep =
+          prepareBackupFactory.createPrepareBackupStep(dm, member, cache, recipients,
+              prepareBackupFactory, properties);
+      existingDataStores = prepareBackupStep.send();
       abort = false;
     } finally {
       if (abort) {
-        new AbortBackupStep(dm, dm.getId(), dm.getCache(), recipients, abortBackupFactory)
+        abortBackupFactory.createAbortBackupStep(dm, member, cache, recipients, abortBackupFactory)
             .send();
         successfulMembers = Collections.emptyMap();
       } else {
-        successfulMembers = new FinishBackupStep(dm, dm.getId(), dm.getCache(), recipients,
-            finishBackupFactory).send();
+        successfulMembers =
+            finishBackupFactory.createFinishBackupStep(dm, member, cache, recipients,
+                finishBackupFactory).send();
       }
     }
     return new BackupDataStoreResult(existingDataStores, successfulMembers);
   }
+
+  interface MissingPersistentMembersProvider {
+    Set<PersistentID> getMissingPersistentMembers(DistributionManager dm);
+  }
+
+  private static class DefaultMissingPersistentMembersProvider
+      implements MissingPersistentMembersProvider {
+    public Set<PersistentID> getMissingPersistentMembers(DistributionManager dm) {
+      return AdminDistributedSystemImpl.getMissingPersistentMembers(dm);
+    }
+  }
 }
diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/backup/FinishBackupFactory.java b/geode-core/src/main/java/org/apache/geode/internal/cache/backup/FinishBackupFactory.java
@@ -42,4 +42,10 @@ BackupResponse createBackupResponse(InternalDistributedMember sender,
       HashSet<PersistentID> persistentIds) {
     return new BackupResponse(sender, persistentIds);
   }
+
+  FinishBackupStep createFinishBackupStep(DistributionManager dm, InternalDistributedMember member,
+      InternalCache cache, Set<InternalDistributedMember> recipients,
+      FinishBackupFactory finishBackupFactory) {
+    return new FinishBackupStep(dm, member, cache, recipients, finishBackupFactory);
+  }
 }
diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/backup/FlushToDiskFactory.java b/geode-core/src/main/java/org/apache/geode/internal/cache/backup/FlushToDiskFactory.java
@@ -40,4 +40,10 @@ FlushToDiskResponse createResponse(InternalDistributedMember sender) {
     return new FlushToDiskResponse(sender);
   }
 
+  FlushToDiskStep createFlushToDiskStep(DistributionManager dm, InternalDistributedMember member,
+      InternalCache cache, Set<InternalDistributedMember> recipients,
+      FlushToDiskFactory flushToDiskFactory) {
+    return new FlushToDiskStep(dm, member, cache, recipients, flushToDiskFactory);
+  }
+
 }
diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/backup/PrepareBackupFactory.java b/geode-core/src/main/java/org/apache/geode/internal/cache/backup/PrepareBackupFactory.java
@@ -50,6 +50,13 @@ BackupResponse createBackupResponse(InternalDistributedMember sender,
     return new BackupResponse(sender, persistentIds);
   }
 
+  PrepareBackupStep createPrepareBackupStep(DistributionManager dm,
+      InternalDistributedMember member,
+      InternalCache cache, Set<InternalDistributedMember> recipients,
+      PrepareBackupFactory prepareBackupFactory, Properties properties) {
+    return new PrepareBackupStep(dm, member, cache, recipients, prepareBackupFactory, properties);
+  }
+
   private String cleanSpecialCharacters(String string) {
     return string.replaceAll("[^\\w]+", "_");
   }

diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/BackupStatusImpl.java b/geode-core/src/main/java/org/apache/geode/management/internal/BackupStatusImpl.java
@@ -24,8 +24,6 @@
 
 /**
  * Holds the result of a backup operation.
- *
- *
  */
 public class BackupStatusImpl implements BackupStatus, Serializable {
   private static final long serialVersionUID = 3704172840296221840L;
@@ -35,15 +33,22 @@ public class BackupStatusImpl implements BackupStatus, Serializable {
 
   public BackupStatusImpl(Map<DistributedMember, Set<PersistentID>> backedUpDiskStores,
       Set<PersistentID> offlineDiskStores) {
-    super();
+    if (backedUpDiskStores == null) {
+      throw new IllegalArgumentException("backedUpDiskStores must not be null");
+    }
+    if (offlineDiskStores == null) {
+      throw new IllegalArgumentException("offlineDiskStores must not be null");
+    }
     this.backedUpDiskStores = backedUpDiskStores;
     this.offlineDiskStores = offlineDiskStores;
   }
 
+  @Override
   public Map<DistributedMember, Set<PersistentID>> getBackedUpDiskStores() {
     return backedUpDiskStores;
   }
 
+  @Override
   public Set<PersistentID> getOfflineDiskStores() {
     return offlineDiskStores;
   }
@@ -53,7 +58,4 @@ public String toString() {
     return "BackupStatus[backedUpDiskStores=" + backedUpDiskStores + ", offlineDiskStores="
         + offlineDiskStores + "]";
   }
-
-
-
 }
diff --git a/...ore/src/main/java/org/apache/geode/management/internal/beans/DistributedSystemBridge.java b/...ore/src/main/java/org/apache/geode/management/internal/beans/DistributedSystemBridge.java
@@ -477,7 +477,7 @@ public boolean removeGatewayReceiverFromSystem(ObjectName objectName, GatewayRec
    */
   public DiskBackupStatus backupAllMembers(String targetDirPath, String baselineDirPath) {
     BackupStatus result =
-        new BackupOperation().backupAllMembers(dm, targetDirPath, baselineDirPath);
+        new BackupOperation(dm, dm.getCache()).backupAllMembers(targetDirPath, baselineDirPath);
     DiskBackupStatusImpl diskBackupStatus = new DiskBackupStatusImpl();
     diskBackupStatus.generateBackedUpDiskStores(result.getBackedUpDiskStores());
     diskBackupStatus.generateOfflineDiskStores(result.getOfflineDiskStores());

diff --git a/...c/main/java/org/apache/geode/management/internal/cli/commands/BackupDiskStoreCommand.java b/...c/main/java/org/apache/geode/management/internal/cli/commands/BackupDiskStoreCommand.java
@@ -60,9 +60,10 @@ public Result backupDiskStore(
       BackupStatus backupStatus;
 
       if (baselineDir != null && !baselineDir.isEmpty()) {
-        backupStatus = new BackupOperation().backupAllMembers(dm, targetDir, baselineDir);
+        backupStatus =
+            new BackupOperation(dm, dm.getCache()).backupAllMembers(targetDir, baselineDir);
       } else {
-        backupStatus = new BackupOperation().backupAllMembers(dm, targetDir, null);
+        backupStatus = new BackupOperation(dm, dm.getCache()).backupAllMembers(targetDir, null);
       }
 
       Map<DistributedMember, Set<PersistentID>> backedupMemberDiskstoreMap =

diff --git a/geode-core/src/test/java/org/apache/geode/internal/cache/backup/BackupDistributedTest.java b/geode-core/src/test/java/org/apache/geode/internal/cache/backup/BackupDistributedTest.java
@@ -700,8 +700,9 @@ private File[] getDiskDirs(VM vm, String dsName) {
   private BackupStatus backupMember(final VM vm) {
     return vm.invoke("backup", () -> {
       try {
-        return new BackupOperation().backupAllMembers(getCache().getDistributionManager(),
-            backupBaseDir.toString(), null);
+        return new BackupOperation(getCache().getDistributionManager(), getCache())
+            .backupAllMembers(
+                backupBaseDir.toString(), null);
       } catch (ManagementException e) {
         throw new RuntimeException(e);
       }