geode-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From aging...@apache.org
Subject [geode] branch develop updated: GEODE-3800: Replace BackupManager with BackupService (#1372)
Date Thu, 01 Feb 2018 18:26:44 GMT
This is an automated email from the ASF dual-hosted git repository.

agingade pushed a commit to branch develop
in repository https://gitbox.apache.org/repos/asf/geode.git


The following commit(s) were added to refs/heads/develop by this push:
     new 0f6e09c  GEODE-3800: Replace BackupManager with BackupService (#1372)
0f6e09c is described below

commit 0f6e09c4a162dc2fec411e7c6267e9d1de8c697e
Author: agingade <agingade@pivotal.io>
AuthorDate: Thu Feb 1 10:26:40 2018 -0800

    GEODE-3800: Replace BackupManager with BackupService (#1372)
    
    * GEODE-3800: Convert backups into a service
---
 .../geode/internal/cache/DiskStoreFactoryImpl.java |  4 +-
 .../apache/geode/internal/cache/DiskStoreImpl.java | 11 ++-
 .../geode/internal/cache/GemFireCacheImpl.java     | 23 ++----
 .../apache/geode/internal/cache/InternalCache.java |  8 +-
 .../org/apache/geode/internal/cache/Oplog.java     |  4 +-
 .../internal/cache/PartitionedRegionDataStore.java |  8 +-
 .../{BackupManager.java => BackupService.java}     | 68 +++++++++-------
 .../geode/internal/cache/backup/BackupTask.java    |  3 +-
 .../cache/backup/FileSystemBackupDestination.java  |  2 +-
 .../geode/internal/cache/backup/FinishBackup.java  |  4 +-
 .../geode/internal/cache/backup/PrepareBackup.java |  4 +-
 .../cache/backup/TemporaryBackupFiles.java         |  2 +-
 .../internal/cache/xmlcache/CacheCreation.java     | 14 +---
 .../geode/internal/i18n/LocalizedStrings.java      |  2 +-
 .../internal/beans/MemberMBeanBridge.java          |  9 +-
 .../cache/DiskStoreImplIntegrationTest.java        |  4 +-
 .../cache/backup/BackupIntegrationTest.java        | 35 +++-----
 .../internal/cache/backup/BackupServiceTest.java   | 95 ++++++++++++++++++++++
 .../cache/backup/FinishBackupRequestTest.java      |  6 +-
 .../beans/DistributedSystemBridgeJUnitTest.java    | 25 +++---
 20 files changed, 196 insertions(+), 135 deletions(-)

diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/DiskStoreFactoryImpl.java
b/geode-core/src/main/java/org/apache/geode/internal/cache/DiskStoreFactoryImpl.java
index ce52220..672272a 100755
--- a/geode-core/src/main/java/org/apache/geode/internal/cache/DiskStoreFactoryImpl.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/cache/DiskStoreFactoryImpl.java
@@ -21,7 +21,7 @@ import org.apache.geode.GemFireIOException;
 import org.apache.geode.cache.DiskStore;
 import org.apache.geode.cache.DiskStoreFactory;
 import org.apache.geode.distributed.internal.ResourceEvent;
-import org.apache.geode.internal.cache.backup.BackupManager;
+import org.apache.geode.internal.cache.backup.BackupService;
 import org.apache.geode.internal.cache.xmlcache.CacheCreation;
 import org.apache.geode.internal.cache.xmlcache.CacheXml;
 import org.apache.geode.internal.cache.xmlcache.DiskStoreAttributesCreation;
@@ -160,7 +160,7 @@ public class DiskStoreFactoryImpl implements DiskStoreFactory {
     // member depends on state that goes into this disk store
     // that isn't backed up.
     if (this.cache instanceof GemFireCacheImpl) {
-      BackupManager backup = this.cache.getBackupManager();
+      BackupService backup = this.cache.getBackupService();
       if (backup != null) {
         backup.waitForBackup();
       }
diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/DiskStoreImpl.java b/geode-core/src/main/java/org/apache/geode/internal/cache/DiskStoreImpl.java
index 5aa153a..f0959b8 100644
--- a/geode-core/src/main/java/org/apache/geode/internal/cache/DiskStoreImpl.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/cache/DiskStoreImpl.java
@@ -56,7 +56,6 @@ import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.concurrent.atomic.AtomicLong;
 import java.util.concurrent.atomic.AtomicReference;
-import java.util.concurrent.locks.Lock;
 import java.util.concurrent.locks.ReentrantLock;
 import java.util.concurrent.locks.ReentrantReadWriteLock;
 import java.util.concurrent.locks.ReentrantReadWriteLock.ReadLock;
@@ -89,7 +88,7 @@ import org.apache.geode.distributed.internal.membership.InternalDistributedMembe
 import org.apache.geode.i18n.StringId;
 import org.apache.geode.internal.Version;
 import org.apache.geode.internal.cache.ExportDiskRegion.ExportWriter;
-import org.apache.geode.internal.cache.backup.BackupManager;
+import org.apache.geode.internal.cache.backup.BackupService;
 import org.apache.geode.internal.cache.entries.DiskEntry;
 import org.apache.geode.internal.cache.entries.DiskEntry.Helper.ValueWrapper;
 import org.apache.geode.internal.cache.entries.DiskEntry.RecoveredEntry;
@@ -1995,7 +1994,7 @@ public class DiskStoreImpl implements DiskStore {
       try {
         List<Path> backupDirectories = Files.list(directoryHolder.getDir().toPath())
             .filter((path) -> path.getFileName().toString()
-                .startsWith(BackupManager.DATA_STORES_TEMPORARY_DIRECTORY))
+                .startsWith(BackupService.DATA_STORES_TEMPORARY_DIRECTORY))
             .filter(p -> Files.isDirectory(p)).collect(Collectors.toList());
         for (Path backupDirectory : backupDirectories) {
           try {
@@ -3989,7 +3988,7 @@ public class DiskStoreImpl implements DiskStore {
 
   /**
    * Lock the disk store to prevent updates. This is the first step of the backup process.
Once all
-   * disk stores on all members are locked, we still move on to startBackup.
+   * disk stores on all members are locked, we still move on to prepareBackup.
    */
   public void lockStoreBeforeBackup() {
     // This will prevent any region level operations like
@@ -4029,8 +4028,8 @@ public class DiskStoreImpl implements DiskStore {
   }
 
   public DiskStoreBackup getInProgressBackup() {
-    BackupManager backupManager = cache.getBackupManager();
-    return backupManager == null ? null : backupManager.getBackupForDiskStore(this);
+    BackupService backupService = cache.getBackupService();
+    return backupService.getBackupForDiskStore(this);
   }
 
   public Collection<DiskRegionView> getKnown() {
diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java
b/geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java
index 3aed22b..5802410 100755
--- a/geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java
@@ -69,7 +69,6 @@ import java.util.concurrent.ThreadPoolExecutor;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicInteger;
-import java.util.concurrent.atomic.AtomicReference;
 import java.util.function.BiPredicate;
 import java.util.function.Supplier;
 import java.util.regex.Pattern;
@@ -175,7 +174,7 @@ import org.apache.geode.i18n.LogWriterI18n;
 import org.apache.geode.internal.Assert;
 import org.apache.geode.internal.ClassPathLoader;
 import org.apache.geode.internal.SystemTimer;
-import org.apache.geode.internal.cache.backup.BackupManager;
+import org.apache.geode.internal.cache.backup.BackupService;
 import org.apache.geode.internal.cache.control.InternalResourceManager;
 import org.apache.geode.internal.cache.control.InternalResourceManager.ResourceType;
 import org.apache.geode.internal.cache.control.ResourceAdvisor;
@@ -504,7 +503,7 @@ public class GemFireCacheImpl implements InternalCache, InternalClientCache,
Has
 
   private final InternalResourceManager resourceManager;
 
-  private final AtomicReference<BackupManager> backupManager = new AtomicReference<>();
+  private final BackupService backupService;
 
   private HeapEvictor heapEvictor = null;
 
@@ -970,6 +969,7 @@ public class GemFireCacheImpl implements InternalCache, InternalClientCache,
Has
       this.diskMonitor = new DiskStoreMonitor(system.getConfig().getLogFile());
 
       addRegionEntrySynchronizationListener(new GatewaySenderQueueEntrySynchronizationListener());
+      backupService = new BackupService(this);
     } // synchronized
   }
 
@@ -4366,22 +4366,9 @@ public class GemFireCacheImpl implements InternalCache, InternalClientCache,
Has
     return Collections.unmodifiableList(this.backupFiles);
   }
 
-  public BackupManager startBackup(InternalDistributedMember sender) throws IOException {
-    BackupManager manager = new BackupManager(sender, this);
-    if (!this.backupManager.compareAndSet(null, manager)) {
-      throw new IOException("Backup already in progress");
-    }
-    manager.validateRequestingAdmin();
-    return manager;
-  }
-
   @Override
-  public void clearBackupManager() {
-    this.backupManager.set(null);
-  }
-
-  public BackupManager getBackupManager() {
-    return this.backupManager.get();
+  public BackupService getBackupService() {
+    return backupService;
   }
 
   // TODO make this a simple int guarded by riWaiters and get rid of the double-check
diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/InternalCache.java b/geode-core/src/main/java/org/apache/geode/internal/cache/InternalCache.java
index 562aaa7..378058c 100644
--- a/geode-core/src/main/java/org/apache/geode/internal/cache/InternalCache.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/cache/InternalCache.java
@@ -52,7 +52,7 @@ import org.apache.geode.distributed.internal.DistributionManager;
 import org.apache.geode.distributed.internal.InternalDistributedSystem;
 import org.apache.geode.distributed.internal.membership.InternalDistributedMember;
 import org.apache.geode.internal.SystemTimer;
-import org.apache.geode.internal.cache.backup.BackupManager;
+import org.apache.geode.internal.cache.backup.BackupService;
 import org.apache.geode.internal.cache.control.InternalResourceManager;
 import org.apache.geode.internal.cache.control.ResourceAdvisor;
 import org.apache.geode.internal.cache.event.EventTrackerExpiryTask;
@@ -186,8 +186,6 @@ public interface InternalCache extends Cache, Extensible<Cache>,
CacheTime {
 
   long cacheTimeMillis();
 
-  void clearBackupManager();
-
   URL getCacheXmlURL();
 
   List<File> getBackupFiles();
@@ -210,8 +208,6 @@ public interface InternalCache extends Cache, Extensible<Cache>,
CacheTime {
 
   boolean getPdxReadSerializedByAnyGemFireServices();
 
-  BackupManager getBackupManager();
-
   void setDeclarativeCacheConfig(CacheConfig cacheConfig);
 
   void initializePdxRegistry();
@@ -233,7 +229,7 @@ public interface InternalCache extends Cache, Extensible<Cache>,
CacheTime {
   <K, V> Region<K, V> basicCreateRegion(String name, RegionAttributes<K, V>
attrs)
       throws RegionExistsException, TimeoutException;
 
-  BackupManager startBackup(InternalDistributedMember sender) throws IOException;
+  BackupService getBackupService();
 
   Throwable getDisconnectCause();
 
diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/Oplog.java b/geode-core/src/main/java/org/apache/geode/internal/cache/Oplog.java
index a5155dc..35fb05f 100755
--- a/geode-core/src/main/java/org/apache/geode/internal/cache/Oplog.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/cache/Oplog.java
@@ -81,7 +81,7 @@ import org.apache.geode.internal.cache.DiskInitFile.DiskRegionFlag;
 import org.apache.geode.internal.cache.DiskStoreImpl.OplogCompactor;
 import org.apache.geode.internal.cache.DiskStoreImpl.OplogEntryIdSet;
 import org.apache.geode.internal.cache.DistributedRegion.DiskPosition;
-import org.apache.geode.internal.cache.backup.BackupManager;
+import org.apache.geode.internal.cache.backup.BackupService;
 import org.apache.geode.internal.cache.entries.DiskEntry;
 import org.apache.geode.internal.cache.entries.DiskEntry.Helper.Flushable;
 import org.apache.geode.internal.cache.entries.DiskEntry.Helper.ValueWrapper;
@@ -5723,7 +5723,7 @@ public class Oplog implements CompactableOplog, Flushable {
 
   public void deleteCRF() {
     oplogSet.crfDelete(this.oplogId);
-    BackupManager backupManager = getInternalCache().getBackupManager();
+    BackupService backupService = getInternalCache().getBackupService();
     DiskStoreBackup inProgressBackup = getParent().getInProgressBackup();
     if (inProgressBackup == null || !inProgressBackup.deferCrfDelete(this)) {
       deleteCRFFileOnly();
diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegionDataStore.java
b/geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegionDataStore.java
index b3af612..fc49392 100644
--- a/geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegionDataStore.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegionDataStore.java
@@ -77,7 +77,7 @@ import org.apache.geode.internal.cache.BucketRegion.RawValue;
 import org.apache.geode.internal.cache.LocalRegion.RegionPerfStats;
 import org.apache.geode.internal.cache.PartitionedRegion.BucketLock;
 import org.apache.geode.internal.cache.PartitionedRegion.SizeEntry;
-import org.apache.geode.internal.cache.backup.BackupManager;
+import org.apache.geode.internal.cache.backup.BackupService;
 import org.apache.geode.internal.cache.execute.BucketMovedException;
 import org.apache.geode.internal.cache.execute.FunctionStats;
 import org.apache.geode.internal.cache.execute.PartitionedRegionFunctionResultSender;
@@ -1676,9 +1676,9 @@ public class PartitionedRegionDataStore implements HasCachePerfStats
{
    * target member.
    */
   private void waitForInProgressBackup() {
-    BackupManager backupManager = getPartitionedRegion().getGemFireCache().getBackupManager();
-    if (getPartitionedRegion().getDataPolicy().withPersistence() && backupManager
!= null) {
-      backupManager.waitForBackup();
+    BackupService backupService = getPartitionedRegion().getGemFireCache().getBackupService();
+    if (getPartitionedRegion().getDataPolicy().withPersistence()) {
+      backupService.waitForBackup();
     }
 
   }
diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/backup/BackupManager.java
b/geode-core/src/main/java/org/apache/geode/internal/cache/backup/BackupService.java
similarity index 69%
rename from geode-core/src/main/java/org/apache/geode/internal/cache/backup/BackupManager.java
rename to geode-core/src/main/java/org/apache/geode/internal/cache/backup/BackupService.java
index 7f55770..1bb463e 100644
--- a/geode-core/src/main/java/org/apache/geode/internal/cache/backup/BackupManager.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/cache/backup/BackupService.java
@@ -15,6 +15,7 @@
 package org.apache.geode.internal.cache.backup;
 
 import java.io.File;
+import java.io.IOException;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
@@ -24,11 +25,11 @@ import java.util.concurrent.Executors;
 import java.util.concurrent.Future;
 import java.util.concurrent.ThreadFactory;
 import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.atomic.AtomicReference;
 
 import org.apache.logging.log4j.Logger;
 
 import org.apache.geode.cache.persistence.PersistentID;
-import org.apache.geode.distributed.internal.DistributionManager;
 import org.apache.geode.distributed.internal.MembershipListener;
 import org.apache.geode.distributed.internal.membership.InternalDistributedMember;
 import org.apache.geode.internal.cache.DiskStoreBackup;
@@ -37,33 +38,31 @@ import org.apache.geode.internal.cache.InternalCache;
 import org.apache.geode.internal.logging.LogService;
 import org.apache.geode.internal.logging.LoggingThreadGroup;
 
-public class BackupManager {
+public class BackupService {
   Logger logger = LogService.getLogger();
 
   public static final String DATA_STORES_TEMPORARY_DIRECTORY = "backupTemp_";
   private final ExecutorService executor;
   private final MembershipListener membershipListener = new BackupMembershipListener();
   private final InternalCache cache;
-  private final InternalDistributedMember sender;
 
-  private BackupTask task;
-  private Future<HashSet<PersistentID>> taskFuture;
+  private transient Future<HashSet<PersistentID>> taskFuture;
 
+  final AtomicReference<BackupTask> currentTask = new AtomicReference<>();
 
-  public BackupManager(InternalDistributedMember sender, InternalCache cache) {
+  public BackupService(InternalCache cache) {
     this.cache = cache;
-    this.sender = sender;
     executor = createExecutor();
   }
 
   private ExecutorService createExecutor() {
-    LoggingThreadGroup group = LoggingThreadGroup.createThreadGroup("BackupManager Thread",
logger);
+    LoggingThreadGroup group = LoggingThreadGroup.createThreadGroup("BackupService Thread",
logger);
     ThreadFactory threadFactory = new ThreadFactory() {
       private final AtomicInteger threadId = new AtomicInteger();
 
       public Thread newThread(final Runnable command) {
         Thread thread =
-            new Thread(group, command, "BackupManagerThread" + this.threadId.incrementAndGet());
+            new Thread(group, command, "BackupServiceThread" + this.threadId.incrementAndGet());
         thread.setDaemon(true);
         return thread;
       }
@@ -71,16 +70,26 @@ public class BackupManager {
     return Executors.newSingleThreadExecutor(threadFactory);
   }
 
-  public void startBackup() {
-    task = new BackupTask(cache);
-    taskFuture = executor.submit(task::backup);
-  }
-
-  public HashSet<PersistentID> getDiskStoreIdsToBackup() throws InterruptedException
{
-    return task.awaitLockAcquisition();
+  public HashSet<PersistentID> prepareBackup(InternalDistributedMember sender)
+      throws IOException, InterruptedException {
+    validateRequestingAdmin(sender);
+    BackupTask backupTask = new BackupTask(cache);
+    if (!currentTask.compareAndSet(null, backupTask)) {
+      throw new IOException("Another backup already in progress");
+    }
+    taskFuture = executor.submit(() -> backupTask.backup());
+    return backupTask.awaitLockAcquisition();
   }
 
-  public HashSet<PersistentID> doBackup(File targetDir, File baselineDir, boolean abort)
{
+  public HashSet<PersistentID> doBackup(File targetDir, File baselineDir, boolean abort)
+      throws IOException {
+    BackupTask task = currentTask.get();
+    if (task == null) {
+      if (abort) {
+        return new HashSet<>();
+      }
+      throw new IOException("No backup currently in progress");
+    }
     task.notifyOtherMembersReady(targetDir, baselineDir, abort);
 
     HashSet<PersistentID> result;
@@ -88,35 +97,38 @@ public class BackupManager {
       result = taskFuture.get();
     } catch (InterruptedException | ExecutionException e) {
       result = new HashSet<>();
+    } finally {
+      cleanup();
     }
     return result;
   }
 
   public void waitForBackup() {
-    task.waitForBackup();
+    BackupTask task = currentTask.get();
+    if (task != null) {
+      task.waitTillBackupFilesAreCopiedToTemporaryLocation();
+    }
   }
 
   public DiskStoreBackup getBackupForDiskStore(DiskStoreImpl diskStore) {
-    return task.getBackupForDiskStore(diskStore);
+    BackupTask task = currentTask.get();
+    return task == null ? null : task.getBackupForDiskStore(diskStore);
   }
 
-  public void validateRequestingAdmin() {
+  void validateRequestingAdmin(InternalDistributedMember sender) {
     // We need to watch for pure admin guys that depart. this allMembershipListener set
     // looks like it should receive those events.
-    Set allIds = getDistributionManager().addAllMembershipListenerAndGetAllIds(membershipListener);
+    Set allIds =
+        cache.getDistributionManager().addAllMembershipListenerAndGetAllIds(membershipListener);
     if (!allIds.contains(sender)) {
       cleanup();
       throw new IllegalStateException("The admin member requesting a backup has already departed");
     }
   }
 
-  private void cleanup() {
-    getDistributionManager().removeAllMembershipListener(membershipListener);
-    cache.clearBackupManager();
-  }
-
-  private DistributionManager getDistributionManager() {
-    return cache.getInternalDistributedSystem().getDistributionManager();
+  void cleanup() {
+    cache.getDistributionManager().removeAllMembershipListener(membershipListener);
+    currentTask.set(null);
   }
 
   private class BackupMembershipListener implements MembershipListener {
diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/backup/BackupTask.java
b/geode-core/src/main/java/org/apache/geode/internal/cache/backup/BackupTask.java
index f984c4c..29753a8 100644
--- a/geode-core/src/main/java/org/apache/geode/internal/cache/backup/BackupTask.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/cache/backup/BackupTask.java
@@ -192,7 +192,7 @@ public class BackupTask {
     return isCancelled;
   }
 
-  void waitForBackup() {
+  void waitTillBackupFilesAreCopiedToTemporaryLocation() {
     try {
       allowDestroys.await();
     } catch (InterruptedException e) {
@@ -207,7 +207,6 @@ public class BackupTask {
       temporaryFiles.cleanupFiles();
     }
     releaseBackupLocks();
-    cache.clearBackupManager();
   }
 
   private void releaseBackupLocks() {
diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/backup/FileSystemBackupDestination.java
b/geode-core/src/main/java/org/apache/geode/internal/cache/backup/FileSystemBackupDestination.java
index 2955d6e..a845766 100644
--- a/geode-core/src/main/java/org/apache/geode/internal/cache/backup/FileSystemBackupDestination.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/cache/backup/FileSystemBackupDestination.java
@@ -61,7 +61,7 @@ public class FileSystemBackupDestination implements BackupDestination {
   }
 
   private void writeReadMe() throws IOException {
-    String text = LocalizedStrings.BackupManager_README.toLocalizedString();
+    String text = LocalizedStrings.BackupService_README.toLocalizedString();
     Files.write(backupDir.resolve(README_FILE), text.getBytes());
   }
 
diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/backup/FinishBackup.java
b/geode-core/src/main/java/org/apache/geode/internal/cache/backup/FinishBackup.java
index f9d5813..ce33042 100644
--- a/geode-core/src/main/java/org/apache/geode/internal/cache/backup/FinishBackup.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/cache/backup/FinishBackup.java
@@ -37,10 +37,10 @@ class FinishBackup {
 
   HashSet<PersistentID> run() throws IOException {
     HashSet<PersistentID> persistentIds;
-    if (cache == null || cache.getBackupManager() == null) {
+    if (cache == null) {
       persistentIds = new HashSet<>();
     } else {
-      persistentIds = cache.getBackupManager().doBackup(targetDir, baselineDir, abort);
+      persistentIds = cache.getBackupService().doBackup(targetDir, baselineDir, abort);
     }
     return persistentIds;
   }
diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/backup/PrepareBackup.java
b/geode-core/src/main/java/org/apache/geode/internal/cache/backup/PrepareBackup.java
index c6130d7..19b6bc1 100644
--- a/geode-core/src/main/java/org/apache/geode/internal/cache/backup/PrepareBackup.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/cache/backup/PrepareBackup.java
@@ -36,9 +36,7 @@ class PrepareBackup {
     if (cache == null) {
       persistentIds = new HashSet<>();
     } else {
-      BackupManager backupManager = cache.startBackup(member);
-      backupManager.startBackup();
-      persistentIds = backupManager.getDiskStoreIdsToBackup();
+      persistentIds = cache.getBackupService().prepareBackup(member);
     }
     return persistentIds;
   }
diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/backup/TemporaryBackupFiles.java
b/geode-core/src/main/java/org/apache/geode/internal/cache/backup/TemporaryBackupFiles.java
index 11099b9..79100c9 100644
--- a/geode-core/src/main/java/org/apache/geode/internal/cache/backup/TemporaryBackupFiles.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/cache/backup/TemporaryBackupFiles.java
@@ -53,7 +53,7 @@ class TemporaryBackupFiles {
    */
   static TemporaryBackupFiles create() throws IOException {
     long currentTime = System.currentTimeMillis();
-    String diskStoreDirectoryName = BackupManager.DATA_STORES_TEMPORARY_DIRECTORY + currentTime;
+    String diskStoreDirectoryName = BackupService.DATA_STORES_TEMPORARY_DIRECTORY + currentTime;
     Path temporaryDirectory = Files.createTempDirectory("backup_" + currentTime);
     return new TemporaryBackupFiles(temporaryDirectory, diskStoreDirectoryName);
   }
diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/xmlcache/CacheCreation.java
b/geode-core/src/main/java/org/apache/geode/internal/cache/xmlcache/CacheCreation.java
index 1d262e9..e5a6c99 100755
--- a/geode-core/src/main/java/org/apache/geode/internal/cache/xmlcache/CacheCreation.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/cache/xmlcache/CacheCreation.java
@@ -127,7 +127,7 @@ import org.apache.geode.internal.cache.RegionListener;
 import org.apache.geode.internal.cache.TXEntryStateFactory;
 import org.apache.geode.internal.cache.TXManagerImpl;
 import org.apache.geode.internal.cache.TombstoneService;
-import org.apache.geode.internal.cache.backup.BackupManager;
+import org.apache.geode.internal.cache.backup.BackupService;
 import org.apache.geode.internal.cache.control.InternalResourceManager;
 import org.apache.geode.internal.cache.control.ResourceAdvisor;
 import org.apache.geode.internal.cache.event.EventTrackerExpiryTask;
@@ -1573,11 +1573,6 @@ public class CacheCreation implements InternalCache {
   }
 
   @Override
-  public BackupManager getBackupManager() {
-    throw new UnsupportedOperationException(LocalizedStrings.SHOULDNT_INVOKE.toLocalizedString());
-  }
-
-  @Override
   public void setDeclarativeCacheConfig(final CacheConfig cacheConfig) {
     throw new UnsupportedOperationException(LocalizedStrings.SHOULDNT_INVOKE.toLocalizedString());
   }
@@ -1659,7 +1654,7 @@ public class CacheCreation implements InternalCache {
   }
 
   @Override
-  public BackupManager startBackup(final InternalDistributedMember sender) throws IOException
{
+  public BackupService getBackupService() {
     throw new UnsupportedOperationException(LocalizedStrings.SHOULDNT_INVOKE.toLocalizedString());
   }
 
@@ -2234,11 +2229,6 @@ public class CacheCreation implements InternalCache {
   }
 
   @Override
-  public void clearBackupManager() {
-    throw new UnsupportedOperationException(LocalizedStrings.SHOULDNT_INVOKE.toLocalizedString());
-  }
-
-  @Override
   public URL getCacheXmlURL() {
     throw new UnsupportedOperationException(LocalizedStrings.SHOULDNT_INVOKE.toLocalizedString());
   }
diff --git a/geode-core/src/main/java/org/apache/geode/internal/i18n/LocalizedStrings.java
b/geode-core/src/main/java/org/apache/geode/internal/i18n/LocalizedStrings.java
index ad8fa64..82b2196 100755
--- a/geode-core/src/main/java/org/apache/geode/internal/i18n/LocalizedStrings.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/i18n/LocalizedStrings.java
@@ -6649,7 +6649,7 @@ public class LocalizedStrings {
       new StringId(5028, "Error deserializing values");
   public static final StringId DistributedRegion_INITIALIZED_FROM_DISK = new StringId(5030,
       "Region {0} recovered from the local disk. Old persistent ID: {1}, new persistent ID
{2}");
-  public static final StringId BackupManager_README = new StringId(5031,
+  public static final StringId BackupService_README = new StringId(5031,
       "This directory contains a backup of the persistent data for a single gemfire VM. The
layout is:\n\ndiskstores\n\tA backup of the persistent disk stores in the VM\nuser\n\tAny
files specified by the backup element in the cache.xml file.\nconfig\n\tThe cache.xml and
gemfire.properties for the backed up member.\nrestore.[sh|bat]\n\tA script to restore the
backup.\n\nPlease note that the config is not restored, only the diskstores and user files.");
   public static final StringId PartitionedRegion_MULTIPLE_TARGET_NODE_FOUND_FOR =
       new StringId(5032, "Multiple target nodes found for single hop operation");
diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/beans/MemberMBeanBridge.java
b/geode-core/src/main/java/org/apache/geode/management/internal/beans/MemberMBeanBridge.java
index 94bbabc..69cdbc3 100644
--- a/geode-core/src/main/java/org/apache/geode/management/internal/beans/MemberMBeanBridge.java
+++ b/geode-core/src/main/java/org/apache/geode/management/internal/beans/MemberMBeanBridge.java
@@ -74,7 +74,6 @@ import org.apache.geode.internal.cache.InternalRegion;
 import org.apache.geode.internal.cache.LocalRegion;
 import org.apache.geode.internal.cache.PartitionedRegion;
 import org.apache.geode.internal.cache.PartitionedRegionStats;
-import org.apache.geode.internal.cache.backup.BackupManager;
 import org.apache.geode.internal.cache.control.ResourceManagerStats;
 import org.apache.geode.internal.cache.execute.FunctionServiceStats;
 import org.apache.geode.internal.i18n.LocalizedStrings;
@@ -1010,17 +1009,15 @@ public class MemberMBeanBridge {
 
     } else {
       try {
-        BackupManager manager =
-            cache.startBackup(cache.getInternalDistributedSystem().getDistributedMember());
         boolean abort = true;
         Set<PersistentID> existingDataStores;
         Set<PersistentID> successfulDataStores;
         try {
-          manager.startBackup();
-          existingDataStores = manager.getDiskStoreIdsToBackup();
+          existingDataStores = cache.getBackupService()
+              .prepareBackup(cache.getInternalDistributedSystem().getDistributedMember());
           abort = false;
         } finally {
-          successfulDataStores = manager.doBackup(targetDir, null/* TODO rishi */, abort);
+          successfulDataStores = cache.getBackupService().doBackup(targetDir, null, abort);
         }
         diskBackUpResult = new DiskBackupResult[existingDataStores.size()];
         int j = 0;
diff --git a/geode-core/src/test/java/org/apache/geode/internal/cache/DiskStoreImplIntegrationTest.java
b/geode-core/src/test/java/org/apache/geode/internal/cache/DiskStoreImplIntegrationTest.java
index 8e9cf6d..3c82ad3 100644
--- a/geode-core/src/test/java/org/apache/geode/internal/cache/DiskStoreImplIntegrationTest.java
+++ b/geode-core/src/test/java/org/apache/geode/internal/cache/DiskStoreImplIntegrationTest.java
@@ -34,7 +34,7 @@ import org.apache.geode.cache.CacheFactory;
 import org.apache.geode.cache.DiskStore;
 import org.apache.geode.cache.RegionShortcut;
 import org.apache.geode.distributed.ConfigurationProperties;
-import org.apache.geode.internal.cache.backup.BackupManager;
+import org.apache.geode.internal.cache.backup.BackupService;
 import org.apache.geode.test.junit.categories.IntegrationTest;
 
 @Category(IntegrationTest.class)
@@ -68,7 +68,7 @@ public class DiskStoreImplIntegrationTest {
     List<Path> tempDirs = new ArrayList<>();
     for (File diskDir : diskStore.getDiskDirs()) {
       Path tempDir =
-          diskDir.toPath().resolve(BackupManager.DATA_STORES_TEMPORARY_DIRECTORY + "testing");
+          diskDir.toPath().resolve(BackupService.DATA_STORES_TEMPORARY_DIRECTORY + "testing");
       Files.createDirectories(tempDir);
       tempDirs.add(tempDir);
     }
diff --git a/geode-core/src/test/java/org/apache/geode/internal/cache/backup/BackupIntegrationTest.java
b/geode-core/src/test/java/org/apache/geode/internal/cache/backup/BackupIntegrationTest.java
index 52375e7..30f2693 100644
--- a/geode-core/src/test/java/org/apache/geode/internal/cache/backup/BackupIntegrationTest.java
+++ b/geode-core/src/test/java/org/apache/geode/internal/cache/backup/BackupIntegrationTest.java
@@ -191,10 +191,8 @@ public class BackupIntegrationTest {
       assertNull(region.get(i));
     }
 
-    BackupManager backup =
-        cache.startBackup(cache.getInternalDistributedSystem().getDistributedMember());
-    backup.startBackup();
-    backup.getDiskStoreIdsToBackup();
+    BackupService backup = cache.getBackupService();
+    backup.prepareBackup(cache.getInternalDistributedSystem().getDistributedMember());
     backup.doBackup(backupDir, null, false);
 
     // Put another key to make sure we restore
@@ -241,10 +239,8 @@ public class BackupIntegrationTest {
   public void testBackupEmptyDiskStore() throws Exception {
     createDiskStore();
 
-    BackupManager backup =
-        cache.startBackup(cache.getInternalDistributedSystem().getDistributedMember());
-    backup.startBackup();
-    backup.getDiskStoreIdsToBackup();
+    BackupService backup = cache.getBackupService();
+    backup.prepareBackup(cache.getInternalDistributedSystem().getDistributedMember());
     backup.doBackup(backupDir, null, false);
     assertEquals("No backup files should have been created", Collections.emptyList(),
         Arrays.asList(backupDir.list()));
@@ -258,10 +254,8 @@ public class BackupIntegrationTest {
     // from a backup that doesn't contain this key
     region.put("A", "A");
 
-    BackupManager backup =
-        cache.startBackup(cache.getInternalDistributedSystem().getDistributedMember());
-    backup.startBackup();
-    backup.getDiskStoreIdsToBackup();
+    BackupService backup = cache.getBackupService();
+    backup.prepareBackup(cache.getInternalDistributedSystem().getDistributedMember());
     backup.doBackup(backupDir, null, false);
 
 
@@ -287,15 +281,12 @@ public class BackupIntegrationTest {
       region.put(i, getBytes(i));
     }
 
-    BackupManager backupManager =
-        cache.startBackup(cache.getInternalDistributedSystem().getDistributedMember());
-    backupManager.validateRequestingAdmin();
-    backupManager.startBackup();
-    backupManager.getDiskStoreIdsToBackup();
+    BackupService backupService = cache.getBackupService();
+    backupService.prepareBackup(cache.getInternalDistributedSystem().getDistributedMember());
     final Region theRegion = region;
     final DiskStore theDiskStore = ds;
     CompletableFuture.runAsync(() -> destroyAndCompact(theRegion, theDiskStore));
-    backupManager.doBackup(backupDir, null, false);
+    backupService.doBackup(backupDir, null, false);
 
     cache.close();
     destroyDiskDirs();
@@ -323,11 +314,9 @@ public class BackupIntegrationTest {
     createDiskStore();
     createRegion();
 
-    BackupManager backup =
-        cache.startBackup(cache.getInternalDistributedSystem().getDistributedMember());
-    backup.startBackup();
-    backup.getDiskStoreIdsToBackup();
-    backup.doBackup(backupDir, null, false);
+    BackupService backupService = cache.getBackupService();
+    backupService.prepareBackup(cache.getInternalDistributedSystem().getDistributedMember());
+    backupService.doBackup(backupDir, null, false);
     Collection<File> fileCollection = FileUtils.listFiles(backupDir,
         new RegexFileFilter("BackupIntegrationTest.cache.xml"), DirectoryFileFilter.DIRECTORY);
     assertEquals(1, fileCollection.size());
diff --git a/geode-core/src/test/java/org/apache/geode/internal/cache/backup/BackupServiceTest.java
b/geode-core/src/test/java/org/apache/geode/internal/cache/backup/BackupServiceTest.java
new file mode 100644
index 0000000..a042cad
--- /dev/null
+++ b/geode-core/src/test/java/org/apache/geode/internal/cache/backup/BackupServiceTest.java
@@ -0,0 +1,95 @@
+/*
+ * 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.geode.internal.cache.backup;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.HashSet;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import org.apache.geode.distributed.internal.DistributionManager;
+import org.apache.geode.distributed.internal.InternalDistributedSystem;
+import org.apache.geode.distributed.internal.membership.InternalDistributedMember;
+import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.test.junit.categories.UnitTest;
+
+@Category(UnitTest.class)
+public class BackupServiceTest {
+
+  BackupService backupService;
+
+  DistributionManager distributionManager;
+
+  InternalDistributedMember sender = new InternalDistributedMember("localhost", 5555);
+
+  InternalCache cache;
+
+  @Before
+  public void setUp() throws Exception {
+    cache = mock(InternalCache.class);
+    distributionManager = mock(DistributionManager.class);
+    InternalDistributedSystem distributedSystem = mock(InternalDistributedSystem.class);
+    InternalDistributedMember distributedMember = mock(InternalDistributedMember.class);
+
+    when(cache.getDistributionManager()).thenReturn(distributionManager);
+    when(distributedSystem.getDistributedMember()).thenReturn(distributedMember);
+    when(cache.getInternalDistributedSystem()).thenReturn(distributedSystem);
+    when(distributedMember.toString()).thenReturn("member");
+    when(distributionManager.addAllMembershipListenerAndGetAllIds(any()))
+        .thenReturn(new HashSet<>(Arrays.asList(sender)));
+
+    backupService = new BackupService(cache);
+  }
+
+  @Test
+  public void throwsExceptionWhenBackupRequesterHasLeftDistributedSystem() {
+    InternalDistributedMember oldSender = new InternalDistributedMember("localhost", 5556);
+    assertThatThrownBy(() -> backupService.validateRequestingAdmin(oldSender))
+        .isInstanceOf(IllegalStateException.class);
+  }
+
+  @Test
+  public void startBackupThrowsExceptionWhenAnotherBackupInProgress() throws Exception {
+    BackupTask backupTask = mock(BackupTask.class);
+    backupService.currentTask.set(backupTask);
+    assertThatThrownBy(() -> backupService.prepareBackup(sender)).isInstanceOf(IOException.class);
+  }
+
+  @Test
+  public void doBackupThrowsExceptionWhenNoBackupInProgress() throws Exception {
+    assertThatThrownBy(() -> backupService.doBackup(null, null, false))
+        .isInstanceOf(IOException.class);
+  }
+
+  @Test
+  public void doBackupAbortsWithEmptyPersistentIds() throws Exception {
+    assertThat(backupService.doBackup(null, null, true).size()).isEqualTo(0);
+  }
+
+  @Test
+  public void prepareBackupReturnsEmptyPersistentIdsWhenBackupNotInProgress() throws Exception
{
+    assertThat(backupService.prepareBackup(sender).size()).isEqualTo(0);
+  }
+
+}
diff --git a/geode-core/src/test/java/org/apache/geode/internal/cache/backup/FinishBackupRequestTest.java
b/geode-core/src/test/java/org/apache/geode/internal/cache/backup/FinishBackupRequestTest.java
index c619de1..53deb7b 100644
--- a/geode-core/src/test/java/org/apache/geode/internal/cache/backup/FinishBackupRequestTest.java
+++ b/geode-core/src/test/java/org/apache/geode/internal/cache/backup/FinishBackupRequestTest.java
@@ -44,7 +44,7 @@ public class FinishBackupRequestTest {
 
   private DistributionManager dm;
   private InternalCache cache;
-  private BackupManager backupManager;
+  private BackupService backupService;
   private int processorId = 79;
   private File targetDir;
   private File baselineDir;
@@ -60,14 +60,14 @@ public class FinishBackupRequestTest {
     // mocks here
     dm = mock(DistributionManager.class);
     cache = mock(InternalCache.class);
-    backupManager = mock(BackupManager.class);
+    backupService = mock(BackupService.class);
     targetDir = mock(File.class);
     baselineDir = mock(File.class);
     abort = false;
 
     when(dm.getCache()).thenReturn(cache);
     when(dm.getDistributionManagerId()).thenReturn(sender);
-    when(cache.getBackupManager()).thenReturn(backupManager);
+    when(cache.getBackupService()).thenReturn(backupService);
 
     sender = mock(InternalDistributedMember.class);
 
diff --git a/geode-core/src/test/java/org/apache/geode/management/internal/beans/DistributedSystemBridgeJUnitTest.java
b/geode-core/src/test/java/org/apache/geode/management/internal/beans/DistributedSystemBridgeJUnitTest.java
index 0ae746e..593103b 100644
--- a/geode-core/src/test/java/org/apache/geode/management/internal/beans/DistributedSystemBridgeJUnitTest.java
+++ b/geode-core/src/test/java/org/apache/geode/management/internal/beans/DistributedSystemBridgeJUnitTest.java
@@ -30,7 +30,7 @@ import org.apache.geode.distributed.internal.DistributionManager;
 import org.apache.geode.distributed.internal.locks.DLockService;
 import org.apache.geode.internal.cache.GemFireCacheImpl;
 import org.apache.geode.internal.cache.backup.BackupDataStoreHelper;
-import org.apache.geode.internal.cache.backup.BackupManager;
+import org.apache.geode.internal.cache.backup.BackupService;
 import org.apache.geode.internal.cache.backup.FinishBackupRequest;
 import org.apache.geode.internal.cache.backup.PrepareBackupRequest;
 import org.apache.geode.internal.cache.persistence.PersistentMemberManager;
@@ -41,16 +41,16 @@ import org.apache.geode.test.junit.categories.UnitTest;
 public class DistributedSystemBridgeJUnitTest {
 
   private GemFireCacheImpl cache;
-  private BackupManager backupManager;
+  private BackupService backupService;
 
   @Before
   public void createCache() throws IOException {
     cache = Fakes.cache();
     PersistentMemberManager memberManager = mock(PersistentMemberManager.class);
-    backupManager = mock(BackupManager.class);
-    when(cache.startBackup(any())).thenReturn(backupManager);
+    backupService = mock(BackupService.class);
+    when(cache.getBackupService()).thenReturn(backupService);
     when(cache.getPersistentMemberManager()).thenReturn(memberManager);
-    when(cache.getBackupManager()).thenReturn(backupManager);
+    when(cache.getBackupService()).thenReturn(backupService);
 
     DLockService dlock = mock(DLockService.class);
     when(dlock.lock(any(), anyLong(), anyLong())).thenReturn(true);
@@ -70,22 +70,21 @@ public class DistributedSystemBridgeJUnitTest {
     DistributedSystemBridge bridge = new DistributedSystemBridge(null, cache);
     bridge.backupAllMembers("/tmp", null);
 
-    InOrder inOrder = inOrder(dm, backupManager);
+    InOrder inOrder = inOrder(dm, backupService);
     inOrder.verify(dm).putOutgoing(isA(PrepareBackupRequest.class));
-    inOrder.verify(backupManager).startBackup();
-    inOrder.verify(backupManager).getDiskStoreIdsToBackup();
+    inOrder.verify(backupService).prepareBackup(any());
     inOrder.verify(dm).putOutgoing(isA(FinishBackupRequest.class));
-    inOrder.verify(backupManager).doBackup(any(), any(), eq(false));
+    inOrder.verify(backupService).doBackup(any(), any(), eq(false));
   }
 
   @Test
   public void testPrepareErrorAbortsBackup() throws Exception {
     DistributionManager dm = cache.getDistributionManager();
     PersistentMemberManager memberManager = mock(PersistentMemberManager.class);
-    BackupManager backupManager = mock(BackupManager.class);
-    when(cache.startBackup(any())).thenReturn(backupManager);
+    BackupService backupService = mock(BackupService.class);
+    when(cache.getBackupService()).thenReturn(backupService);
     when(cache.getPersistentMemberManager()).thenReturn(memberManager);
-    when(cache.getBackupManager()).thenReturn(backupManager);
+    when(cache.getBackupService()).thenReturn(backupService);
     when(dm.putOutgoing(isA(PrepareBackupRequest.class)))
         .thenThrow(new RuntimeException("Fail the prepare"));
 
@@ -98,6 +97,6 @@ public class DistributedSystemBridgeJUnitTest {
     }
 
     verify(dm).putOutgoing(isA(FinishBackupRequest.class));
-    verify(backupManager).doBackup(any(), any(), eq(true));
+    verify(backupService).doBackup(any(), any(), eq(true));
   }
 }

-- 
To stop receiving notification emails like this one, please contact
agingade@apache.org.

Mime
View raw message