From ab81e77790b5c4d2c7792dbcaa725e468db84779 Mon Sep 17 00:00:00 2001 From: BenWhitehead Date: Thu, 14 Sep 2023 14:45:31 -0400 Subject: [PATCH] fix: update RecoveryFileManager to allow distinct files for multiple invocations of equivalent info When creating a new recovery file it is important that distinct recovery files be created always, even if the provided BlobInfo is equivalent to a previously created one. Allocate a UUID, and hash it with goodFastHash(64) before encoding to base64 (url safe). --- .../cloud/storage/RecoveryFileManager.java | 18 +++++++-- .../storage/RecoveryFileManagerTest.java | 40 +++++++++++++++++++ 2 files changed, 55 insertions(+), 3 deletions(-) diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/RecoveryFileManager.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/RecoveryFileManager.java index 4926c899af..1ec06b5f2f 100644 --- a/google-cloud-storage/src/main/java/com/google/cloud/storage/RecoveryFileManager.java +++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/RecoveryFileManager.java @@ -19,8 +19,12 @@ import static com.google.common.base.Preconditions.checkArgument; import com.google.common.collect.ImmutableList; -import com.google.common.primitives.Ints; +import com.google.common.hash.HashCode; +import com.google.common.hash.HashFunction; +import com.google.common.hash.Hasher; +import com.google.common.hash.Hashing; import java.io.IOException; +import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; import java.util.Base64; @@ -28,6 +32,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.UUID; final class RecoveryFileManager { @@ -35,6 +40,8 @@ final class RecoveryFileManager { /** Keep track of active info and file */ private final Map files; + private final HashFunction hashFunction; + /** * Round-robin assign recovery files to the configured volumes. Use this index to keep track of * which volume to assign to next. @@ -45,13 +52,18 @@ private RecoveryFileManager(List volumes) { this.volumes = ImmutableList.copyOf(volumes); this.files = Collections.synchronizedMap(new HashMap<>()); this.nextVolumeIndex = 0; + this.hashFunction = Hashing.goodFastHash(64); } + @SuppressWarnings("UnstableApiUsage") public RecoveryFile newRecoveryFile(BlobInfo info) { int i = getNextVolumeIndex(); RecoveryVolume v = volumes.get(i); - int hashCode = info.hashCode(); - String fileName = Base64.getUrlEncoder().encodeToString(Ints.toByteArray(hashCode)); + UUID uuid = UUID.randomUUID(); + String string = uuid.toString(); + Hasher hasher = hashFunction.newHasher(); + HashCode hash = hasher.putString(string, StandardCharsets.UTF_8).hash(); + String fileName = Base64.getUrlEncoder().encodeToString(hash.asBytes()); Path path = v.basePath.resolve(fileName); RecoveryFile recoveryFile = new RecoveryFile(path, v.sink, () -> files.remove(info)); files.put(info, recoveryFile); diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/RecoveryFileManagerTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/RecoveryFileManagerTest.java index abf68893bb..584ab68b54 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/RecoveryFileManagerTest.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/RecoveryFileManagerTest.java @@ -16,7 +16,10 @@ package com.google.cloud.storage; +import static com.google.cloud.storage.TestUtils.assertAll; +import static com.google.cloud.storage.TestUtils.xxd; import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.Truth.assertWithMessage; import static org.junit.Assert.assertThrows; import com.google.common.collect.ImmutableList; @@ -33,6 +36,7 @@ import java.time.Duration; import java.time.Instant; import java.util.Objects; +import java.util.Random; import java.util.stream.Stream; import org.junit.Rule; import org.junit.Test; @@ -143,4 +147,40 @@ public void fileAssignmentIsRoundRobin() throws IOException { assertThat(parentDirs).isEqualTo(ImmutableSet.of(tempDir1, tempDir2, tempDir3)); } } + + @Test + public void multipleRecoveryFilesForEqualBlobInfoAreAbleToExistConcurrently() throws Exception { + Path tempDir = temporaryFolder.newFolder(testName.getMethodName()).toPath(); + RecoveryFileManager rfm = + RecoveryFileManager.of( + ImmutableList.of(tempDir), + path -> ThroughputSink.logged(path.toAbsolutePath().toString(), clock)); + + BlobInfo info = BlobInfo.newBuilder("bucket", "object").build(); + try (RecoveryFile rf1 = rfm.newRecoveryFile(info); + RecoveryFile rf2 = rfm.newRecoveryFile(info); ) { + + Random rand = new Random(467123); + byte[] bytes1 = DataGenerator.rand(rand).genBytes(7); + byte[] bytes2 = DataGenerator.rand(rand).genBytes(41); + try (WritableByteChannel writer = rf1.writer()) { + writer.write(ByteBuffer.wrap(bytes1)); + } + try (WritableByteChannel writer = rf2.writer()) { + writer.write(ByteBuffer.wrap(bytes2)); + } + + byte[] actual1 = ByteStreams.toByteArray(Files.newInputStream(rf1.getPath())); + byte[] actual2 = ByteStreams.toByteArray(Files.newInputStream(rf2.getPath())); + + String expected1 = xxd(bytes1); + String expected2 = xxd(bytes2); + + String xxd1 = xxd(actual1); + String xxd2 = xxd(actual2); + assertAll( + () -> assertWithMessage("rf1 should contain bytes1").that(xxd1).isEqualTo(expected1), + () -> assertWithMessage("rf2 should contain bytes2").that(xxd2).isEqualTo(expected2)); + } + } }