Revert "Revert "Backup temporary directory rotation fix""

This reverts commit 29a3dcf.

community/kernel/src/test/java/org/neo4j/test/rule/EmbeddedDatabaseRule.java

@@ -71,7 +71,7 @@ protected GraphDatabaseFactory newFactory()
     @Override
     protected GraphDatabaseBuilder newBuilder( GraphDatabaseFactory factory )
     {
-        return factory.newEmbeddedDatabaseBuilder( testDirectory.graphDbDir() );
+        return factory.newEmbeddedDatabaseBuilder( getStoreDir() );
     }
 
     @Override

enterprise/backup/src/main/java/org/neo4j/backup/impl/BackupCopyService.java

@@ -70,6 +70,7 @@ public void moveBackupLocation( Path oldLocation, Path newLocation ) throws IOEx
             {
                 moves.next().move( target );
             }
+            oldLocation.toFile().delete();
         }
         catch ( IOException e )
         {

enterprise/backup/src/main/java/org/neo4j/backup/impl/BackupStrategyWrapper.java

@@ -74,8 +74,7 @@ Fallible<BackupStrategyOutcome> doBackup( OnlineBackupContext onlineBackupContex
         return state;
     }
 
-    private Fallible<BackupStrategyOutcome> performBackupWithoutLifecycle(
-            OnlineBackupContext onlineBackupContext )
+    private Fallible<BackupStrategyOutcome> performBackupWithoutLifecycle( OnlineBackupContext onlineBackupContext )
     {
         Path backupLocation = onlineBackupContext.getResolvedLocationFromName();
         Path userSpecifiedBackupLocation = onlineBackupContext.getResolvedLocationFromName();
@@ -136,8 +135,7 @@ private void clearIdFiles( Path backupLocation )
      * @param onlineBackupContext command line arguments, config etc.
      * @return outcome of full backup
      */
-    private Fallible<BackupStageOutcome> fullBackupWithTemporaryFolderResolutions(
-            OnlineBackupContext onlineBackupContext )
+    private Fallible<BackupStageOutcome> fullBackupWithTemporaryFolderResolutions( OnlineBackupContext onlineBackupContext )
     {
         Path userSpecifiedBackupLocation = onlineBackupContext.getResolvedLocationFromName();
         Path temporaryFullBackupLocation = backupCopyService.findAnAvailableLocationForNewFullBackup( userSpecifiedBackupLocation );
@@ -146,12 +144,12 @@ private Fallible<BackupStageOutcome> fullBackupWithTemporaryFolderResolutions(
         Fallible<BackupStageOutcome> state = backupStrategy.performFullBackup( temporaryFullBackupLocation, config, address );
 
         // NOTE temporaryFullBackupLocation can be equal to desired
-        boolean aBackupAlreadyExisted = userSpecifiedBackupLocation.equals( temporaryFullBackupLocation );
+        boolean backupWasMadeToATemporaryLocation = !userSpecifiedBackupLocation.equals( temporaryFullBackupLocation );
 
         if ( BackupStageOutcome.SUCCESS.equals( state.getState() ) )
         {
             backupRecoveryService.recoverWithDatabase( temporaryFullBackupLocation, pageCache, config );
-            if ( !aBackupAlreadyExisted )
+            if ( backupWasMadeToATemporaryLocation )
             {
                 try
                 {

enterprise/backup/src/test/java/org/neo4j/backup/impl/OnlineBackupCommandCcIT.java

@@ -40,7 +40,10 @@
 import java.nio.file.Paths;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ExecutionException;
 import java.util.concurrent.atomic.AtomicBoolean;
 
 import org.neo4j.causalclustering.ClusterHelper;
@@ -49,6 +52,8 @@
 import org.neo4j.causalclustering.core.consensus.roles.Role;
 import org.neo4j.causalclustering.discovery.Cluster;
 import org.neo4j.causalclustering.discovery.CoreClusterMember;
+import org.neo4j.causalclustering.discovery.IpFamily;
+import org.neo4j.causalclustering.discovery.SharedDiscoveryServiceFactory;
 import org.neo4j.causalclustering.helpers.CausalClusteringTestHelpers;
 import org.neo4j.graphdb.factory.GraphDatabaseSettings;
 import org.neo4j.kernel.configuration.Config;
@@ -64,8 +69,10 @@
 import org.neo4j.util.TestHelpers;
 
 import static java.lang.String.format;
+import static java.util.Collections.emptyMap;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotEquals;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assume.assumeFalse;
 
@@ -269,6 +276,45 @@ public void reportsProgress() throws Exception
         assertTrue( output.contains( "Finished receiving index snapshots" ) );
     }
 
+    @Test
+    public void backupRenamesWork() throws Exception
+    {
+        // given a prexisting backup from a different store
+        String backupName = "preexistingBackup_" + recordFormat;
+        Cluster cluster = startCluster( recordFormat );
+        String firstBackupAddress = CausalClusteringTestHelpers.transactionAddress( clusterLeader( cluster ).database() );
+
+        assertEquals( 0, runBackupToolFromOtherJvmToGetExitCode(
+                "--from", firstBackupAddress,
+                "--cc-report-dir=" + backupDir,
+                "--backup-dir=" + backupDir,
+                "--name=" + backupName ) );
+        DbRepresentation firstDatabaseRepresentation = DbRepresentation.of( clusterLeader( cluster ).database() );
+
+        // and a different database
+        Cluster cluster2 = startCluster2( recordFormat );
+        DbRepresentation secondDatabaseRepresentation = DbRepresentation.of( clusterLeader( cluster2 ).database() );
+        assertNotEquals( firstDatabaseRepresentation, secondDatabaseRepresentation );
+        String secondBackupAddress = CausalClusteringTestHelpers.transactionAddress( clusterLeader( cluster2 ).database() );
+
+        // when backup is performed
+        assertEquals( 0, runBackupToolFromOtherJvmToGetExitCode(
+                "--from", secondBackupAddress,
+                "--cc-report-dir=" + backupDir,
+                "--backup-dir=" + backupDir,
+                "--name=" + backupName ) );
+        cluster2.shutdown();
+
+        // then the new backup has the correct name
+        assertEquals( secondDatabaseRepresentation, getBackupDbRepresentation( backupName, backupDir ) );
+
+        // and the old backup is in a renamed location
+        assertEquals( firstDatabaseRepresentation, getBackupDbRepresentation( backupName + ".err.0", backupDir ) );
+
+        // and the data isn't equal (sanity check)
+        assertNotEquals( firstDatabaseRepresentation, secondDatabaseRepresentation );
+    }
+
     static PrintStream wrapWithNormalOutput( PrintStream normalOutput, PrintStream nullAbleOutput )
     {
         if ( nullAbleOutput == null )
@@ -342,6 +388,19 @@ private Cluster startCluster( String recordFormat ) throws Exception
         return cluster;
     }
 
+    private Cluster startCluster2( String recordFormat ) throws ExecutionException, InterruptedException
+    {
+        Map<String,String> sharedParams = new HashMap<>(  );
+        sharedParams.put( GraphDatabaseSettings.record_format.name(), recordFormat );
+        Cluster cluster =
+                new Cluster( testDirectory.directory( "cluster-b_" + recordFormat ), 3, 0, new SharedDiscoveryServiceFactory(), sharedParams, emptyMap(),
+                sharedParams, emptyMap(),
+                recordFormat, IpFamily.IPV4, false );
+        cluster.start();
+        createSomeData( cluster );
+        return cluster;
+    }
+
     public static DbRepresentation createSomeData( Cluster cluster )
     {
         try

enterprise/backup/src/test/java/org/neo4j/backup/impl/OnlineBackupCommandHaIT.java

@@ -48,6 +48,8 @@
 import org.neo4j.graphdb.Node;
 import org.neo4j.graphdb.RelationshipType;
 import org.neo4j.graphdb.Transaction;
+import org.neo4j.graphdb.factory.GraphDatabaseBuilder;
+import org.neo4j.graphdb.factory.GraphDatabaseFactory;
 import org.neo4j.graphdb.factory.GraphDatabaseSettings;
 import org.neo4j.io.fs.DefaultFileSystemAbstraction;
 import org.neo4j.kernel.configuration.Config;
@@ -66,6 +68,7 @@
 import static org.hamcrest.Matchers.greaterThan;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotEquals;
 import static org.junit.Assert.assertThat;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assume.assumeFalse;
@@ -110,6 +113,19 @@ private static void createSomeData( GraphDatabaseService db )
         }
     }
 
+    private static void createSpecificNodePair( GraphDatabaseService db, String name )
+    {
+        try ( Transaction tx = db.beginTx() )
+        {
+            Node left = db.createNode();
+            left.setProperty( "name", name + "Left" );
+            Node right = db.createNode();
+            right.setProperty( "name", name + "Right" );
+            right.createRelationshipTo( left, RelationshipType.withName( "KNOWS" ) );
+            tx.success();
+        }
+    }
+
     @Before
     public void resetTasks()
     {
@@ -143,15 +159,15 @@ public void makeSureBackupCanBePerformedWithCustomPort() throws Exception
                         "--cc-report-dir=" + backupDir,
                         "--backup-dir=" + backupDir,
                         "--name=" + backupName ) );
-        assertEquals( getDbRepresentation(), getBackupDbRepresentation( backupName ) );
+        assertEquals( DbRepresentation.of( db ), getBackupDbRepresentation( backupName ) );
         createSomeData( db );
         assertEquals(
                 0,
                 runBackupTool( testDirectory.absolutePath(), "--from", "127.0.0.1:" + backupPort,
                         "--cc-report-dir=" + backupDir,
                         "--backup-dir=" + backupDir,
                         "--name=" + backupName ) );
-        assertEquals( getDbRepresentation(), getBackupDbRepresentation( backupName ) );
+        assertEquals( DbRepresentation.of( db ), getBackupDbRepresentation( backupName ) );
     }
 
     @Test
@@ -289,6 +305,48 @@ public void reportsProgress() throws Exception
         assertFalse( output.contains( "Finished receiving index snapshots" ) );
     }
 
+    @Test
+    public void backupRenamesWork() throws Exception
+    {
+        // given a prexisting backup from a different store
+        String backupName = "preexistingBackup_" + recordFormat;
+        int firstBackupPort = PortAuthority.allocatePort();
+        startDb( firstBackupPort );
+        createSpecificNodePair( db, "first" );
+
+        assertEquals( 0, runSameJvm( backupDir, backupName,
+                "--from", "127.0.0.1:" + firstBackupPort,
+                "--cc-report-dir=" + backupDir,
+                "--protocol=common",
+                "--backup-dir=" + backupDir,
+                "--name=" + backupName ) );
+        DbRepresentation firstDatabaseRepresentation = DbRepresentation.of( db );
+
+        // and a different database
+        int secondBackupPort = PortAuthority.allocatePort();
+        GraphDatabaseService db2 = createDb2( secondBackupPort );
+        createSpecificNodePair( db2, "second" );
+        DbRepresentation secondDatabaseRepresentation = DbRepresentation.of( db2 );
+
+        // when backup is performed
+        assertEquals( 0, runSameJvm(backupDir, backupName,
+                "--from", "127.0.0.1:" + secondBackupPort,
+                "--cc-report-dir=" + backupDir,
+                "--backup-dir=" + backupDir,
+                "--protocol=common",
+                "--name=" + backupName ) );
+
+        // then the new backup has the correct name
+        assertEquals( secondDatabaseRepresentation, getBackupDbRepresentation( backupName ) );
+
+        // and the old backup is in a renamed location
+        assertEquals( firstDatabaseRepresentation, getBackupDbRepresentation( backupName + ".err.0" ) );
+
+        // and the data isn't equal (sanity check)
+        assertNotEquals( firstDatabaseRepresentation, secondDatabaseRepresentation );
+        db2.shutdown();
+    }
+
     private void repeatedlyPopulateDatabase( GraphDatabaseService db, AtomicBoolean continueFlagReference )
     {
         while ( continueFlagReference.get() )
@@ -313,7 +371,21 @@ private void createIndexes( GraphDatabaseService db )
         }
     }
 
+    private GraphDatabaseService createDb2( Integer backupPort )
+    {
+        File storeDir = testDirectory.directory("graph-db-2");
+        GraphDatabaseFactory factory = new GraphDatabaseFactory();
+        GraphDatabaseBuilder builder = factory.newEmbeddedDatabaseBuilder( storeDir );
+        builder.setConfig( OnlineBackupSettings.online_backup_server, "0.0.0.0:" + backupPort );
+        return builder.newGraphDatabase();
+    }
+
     private void startDb( Integer backupPort )
+    {
+        startDb( db, backupPort );
+    }
+
+    private void startDb( EmbeddedDatabaseRule db, Integer backupPort )
     {
         db.setConfig( GraphDatabaseSettings.record_format, recordFormat );
         db.setConfig( OnlineBackupSettings.online_backup_enabled, Settings.TRUE );
@@ -333,9 +405,18 @@ private static int runBackupTool( File neo4jHome, String... args )
         return runBackupToolFromOtherJvmToGetExitCode( neo4jHome, args );
     }
 
-    private DbRepresentation getDbRepresentation()
+    private static int runSameJvm( File home, String name, String... args )
     {
-        return DbRepresentation.of( db );
+        try
+        {
+            new OnlineBackupCommandBuilder().withRawArgs( args ).backup( home, name );
+            return 0;
+        }
+        catch ( Exception e )
+        {
+            e.printStackTrace();
+            return 1;
+        }
     }
 
     private DbRepresentation getBackupDbRepresentation( String name )

enterprise/com/src/main/java/org/neo4j/com/storecopy/FileMoveAction.java

@@ -74,6 +74,7 @@ public void move( File toDir, CopyOption... copyOptions ) throws IOException
                 Path resolvedPath = toDir.toPath().resolve( relativePath );
                 Files.createDirectories( resolvedPath.getParent() );
                 Files.copy( originalPath, resolvedPath, copyOptions );
+                Files.delete( originalPath );
             }
 
             @Override

enterprise/com/src/main/java/org/neo4j/com/storecopy/FileMoveProvider.java

@@ -126,7 +126,7 @@ private Stream<FileMoveAction> expandTraverseFiles( File dir, File basePath )
         File base = basePath; // Capture effectively-final base path snapshot.
         Stream<File> files = listing.stream().filter( this::isFile );
         Stream<File> dirs = listing.stream().filter( this::isDirectory );
-        Stream<FileMoveAction> moveFiles = files.map( f -> copyFileCorrectly( f, base ) );
+        Stream<FileMoveAction> moveFiles = files.map( f -> moveFileCorrectly( f, base ) );
         Stream<FileMoveAction> traverseDirectories = dirs.flatMap( d -> traverseForMoving( d, base ) );
         return Stream.concat( moveFiles, traverseDirectories );
     }
@@ -171,7 +171,7 @@ private List<File> listFiles( File dir )
      * contains the logic for handling files between
      * the 2 systems
      */
-    private FileMoveAction copyFileCorrectly( File fileToMove, File basePath )
+    private FileMoveAction moveFileCorrectly( File fileToMove, File basePath )
     {
         if ( fileMoveActionInformer.shouldBeManagedByPageCache( fileToMove.getName() ) )
         {