Skip to content

Commit

Permalink
Changes based on latest comments by kou
Browse files Browse the repository at this point in the history
  • Loading branch information
@felipecrv
felipecrv committed Feb 9, 2024
1 parent 86c551a commit 280a61c
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 19 deletions.
22 changes: 12 additions & 10 deletions cpp/src/arrow/filesystem/azurefs.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<std::chrono::seconds> break_period = {}) {
auto remaining_time_ms = [this]() {
const auto remaing_time = break_or_expires_at_ - SteadyClock::now();
return std::chrono::duration_cast<std::chrono::milliseconds>(remaing_time);
const auto remaining_time = break_or_expires_at_ - SteadyClock::now();
return std::chrono::duration_cast<std::chrono::milliseconds>(remaining_time);
};
#ifndef NDEBUG
if (break_period.HasValue() && !StillValidFor(*break_period)) {
Expand Down Expand Up @@ -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.
Expand All @@ -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) {
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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) {
Expand All @@ -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);
Expand All @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down
16 changes: 7 additions & 9 deletions cpp/src/arrow/filesystem/azurefs_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
Expand Down Expand Up @@ -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() {
Expand All @@ -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);

Expand All @@ -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);
Expand Down Expand Up @@ -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/"));
Expand Down Expand Up @@ -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)

Expand Down

0 comments on commit 280a61c

Please sign in to comment.