From 280a61c7a1d20ce9dbc26e1010b7d293f735054d Mon Sep 17 00:00:00 2001 From: Felipe Oliveira Carvalho Date: Thu, 8 Feb 2024 23:45:59 -0300 Subject: [PATCH] Changes based on latest comments by kou --- cpp/src/arrow/filesystem/azurefs.cc | 22 ++++++++++++---------- cpp/src/arrow/filesystem/azurefs_test.cc | 16 +++++++--------- 2 files changed, 19 insertions(+), 19 deletions(-) diff --git a/cpp/src/arrow/filesystem/azurefs.cc b/cpp/src/arrow/filesystem/azurefs.cc index 82d0bdec0dfff..81fd813962792 100644 --- a/cpp/src/arrow/filesystem/azurefs.cc +++ b/cpp/src/arrow/filesystem/azurefs.cc @@ -1057,8 +1057,8 @@ class LeaseGuard { /// https://learn.microsoft.com/en-us/rest/api/storageservices/lease-blob#outcomes-of-use-attempts-on-blobs-by-lease-state Status Break(Azure::Nullable break_period = {}) { auto remaining_time_ms = [this]() { - const auto remaing_time = break_or_expires_at_ - SteadyClock::now(); - return std::chrono::duration_cast(remaing_time); + const auto remaining_time = break_or_expires_at_ - SteadyClock::now(); + return std::chrono::duration_cast(remaining_time); }; #ifndef NDEBUG if (break_period.HasValue() && !StillValidFor(*break_period)) { @@ -2136,6 +2136,8 @@ class AzureFileSystem::Impl { try { auto src_list_response = src_container_client.ListBlobs(list_blobs_options); if (!src_list_response.Blobs.empty()) { + // Reminder: dest is used here because we're semantically replacing dest + // with src. By deleting src if it's empty just like dest. return Status::IOError("Unable to replace empty container: '", dest.all, "'"); } // Delete the source container now that we know it's empty. @@ -2152,10 +2154,10 @@ class AzureFileSystem::Impl { } try { src_lease_guard.BreakBeforeDeletion(kTimeNeededForContainerDeletion); - src_container_client.DeleteIfExists(options); + src_container_client.Delete(options); src_lease_guard.Forget(); } catch (const Storage::StorageException& exception) { - return ExceptionToStatus(exception, "Failed to delete empty container '", + return ExceptionToStatus(exception, "Failed to delete empty container: '", src.container, "': ", src_container_client.GetUrl()); } } catch (const Storage::StorageException& exception) { @@ -2205,7 +2207,7 @@ class AzureFileSystem::Impl { return CrossContainerMoveNotImplemented(src, dest); } - Status MovePathsWithDataLakeAPI( + Status MovePathWithDataLakeAPI( const DataLake::DataLakeFileSystemClient& src_adlfs_client, const AzureLocation& src, const AzureLocation& dest) { DCHECK(!src.container.empty() && !src.path.empty()); @@ -2319,7 +2321,7 @@ class AzureFileSystem::Impl { return Status::OK(); } - Status MovePathsUsingBlobsAPI(const AzureLocation& src, const AzureLocation& dest) { + Status MovePathUsingBlobsAPI(const AzureLocation& src, const AzureLocation& dest) { DCHECK(!src.container.empty() && !src.path.empty()); DCHECK(!dest.container.empty() && !dest.path.empty()); if (src.container != dest.container) { @@ -2332,7 +2334,7 @@ class AzureFileSystem::Impl { return Status::NotImplemented("The Azure FileSystem is not fully implemented"); } - Status MovePaths(const AzureLocation& src, const AzureLocation& dest) { + Status MovePath(const AzureLocation& src, const AzureLocation& dest) { DCHECK(!src.container.empty() && !src.path.empty()); DCHECK(!dest.container.empty() && !dest.path.empty()); auto src_adlfs_client = GetFileSystemClient(src.container); @@ -2342,10 +2344,10 @@ class AzureFileSystem::Impl { return PathNotFound(src); } if (hns_support == HNSSupport::kEnabled) { - return MovePathsWithDataLakeAPI(src_adlfs_client, src, dest); + return MovePathWithDataLakeAPI(src_adlfs_client, src, dest); } DCHECK_EQ(hns_support, HNSSupport::kDisabled); - return MovePathsUsingBlobsAPI(src, dest); + return MovePathUsingBlobsAPI(src, dest); } Status CopyFile(const AzureLocation& src, const AzureLocation& dest) { @@ -2542,7 +2544,7 @@ Status AzureFileSystem::Move(const std::string& src, const std::string& dest) { if (dest_location.path.empty()) { return impl_->CreateContainerFromPath(src_location, dest_location); } - return impl_->MovePaths(src_location, dest_location); + return impl_->MovePath(src_location, dest_location); } Status AzureFileSystem::CopyFile(const std::string& src, const std::string& dest) { diff --git a/cpp/src/arrow/filesystem/azurefs_test.cc b/cpp/src/arrow/filesystem/azurefs_test.cc index 9e48f03b92cb8..89d54e8d994c6 100644 --- a/cpp/src/arrow/filesystem/azurefs_test.cc +++ b/cpp/src/arrow/filesystem/azurefs_test.cc @@ -1010,7 +1010,7 @@ class TestAzureFileSystem : public ::testing::Test { FileSelector select; select.base_dir = dest_path; select.recursive = false; - // TODO(felipecrv): investigate why this can't be false + // TODO(ARROW-40014): investigate why this can't be false here select.allow_not_found = true; ARROW_ASSIGN_OR_RAISE(auto dest_contents, the_fs->GetFileInfo(select)); if (dest_contents.empty()) { @@ -1163,8 +1163,8 @@ class TestAzureFileSystem : public ::testing::Test { EXPECT_RAISES_WITH_MESSAGE_THAT( NotImplemented, HasCrossContainerNotImplementedMessage(data.container_name, - "a-container/new-subdir"), - fs()->Move(data.container_name, "a-container/new-subdir")); + "missing-container/new-subdir"), + fs()->Move(data.container_name, "missing-container/new-subdir")); } void TestCreateContainerFromPath() { @@ -1185,11 +1185,11 @@ class TestAzureFileSystem : public ::testing::Test { fs()->Move(src_dir_path, "new-container")); } - void TestMovePaths() { + void TestMovePath() { Status st; auto data = SetUpPreexistingData(); // When source doesn't exist. - ASSERT_MOVE("missing-container/src-path", "a-container/dest-path", ENOENT); + ASSERT_MOVE("missing-container/src-path", data.ContainerPath("dest-path"), ENOENT); auto missing_path1 = data.RandomDirectoryPath(rng_); ASSERT_MOVE(missing_path1, "missing-container/path", ENOENT); @@ -1202,8 +1202,7 @@ class TestAzureFileSystem : public ::testing::Test { HasCrossContainerNotImplementedMessage(data.ObjectPath(), "missing-container/path"), fs()->Move(data.ObjectPath(), "missing-container/path")); - GTEST_SKIP() - << "The rest of TestMovePaths is not implemented for non-HNS scenarios"; + GTEST_SKIP() << "The rest of TestMovePath is not implemented for non-HNS scenarios"; } auto adlfs_client = datalake_service_client_->GetFileSystemClient(data.container_name); @@ -1250,7 +1249,6 @@ class TestAzureFileSystem : public ::testing::Test { // "subdir0/file-at-subdir" exists // src is a directory and dest does not exists - CreateDirectory(adlfs_client, "subdir0"); ASSERT_MOVE_OK(data.Path("subdir0"), data.Path("subdir1")); ASSERT_MOVE_OK(data.Path("subdir1/"), data.Path("subdir2")); ASSERT_MOVE_OK(data.Path("subdir2"), data.Path("subdir3/")); @@ -1542,7 +1540,7 @@ TYPED_TEST(TestAzureFileSystemOnAllScenarios, CreateContainerFromPath) { this->TestCreateContainerFromPath(); } -TYPED_TEST(TestAzureFileSystemOnAllScenarios, MovePaths) { this->TestMovePaths(); } +TYPED_TEST(TestAzureFileSystemOnAllScenarios, MovePath) { this->TestMovePath(); } // Tests using Azurite (the local Azure emulator)