GH-38700: [C++][FS][Azure] Implement `DeleteDir()` #38793

kou · 2023-11-20T09:22:30Z

Rationale for this change

DeleteDir() deletes the given directory recursively like other filesystem implementations.

What changes are included in this PR?

Container can be deleted with/without hierarchical namespace support.
Directory can be deleted with hierarchical namespace support.
Directory can't be deleted without hierarchical namespace support. But blobs under the given path can be deleted. So these blobs are deleted and the given virtual directory is also deleted.

Are these changes tested?

Yes.

Are there any user-facing changes?

Yes.

Closes: [C++][FS][Azure] Implement DeleteDir() #38700

github-actions · 2023-11-20T09:22:59Z

⚠️ GitHub issue #38700 has been automatically assigned in GitHub to PR creator.

kou · 2023-11-20T09:30:13Z

@Tom-Newton @felipecrv You may want to review this.

Tom-Newton · 2023-11-20T10:09:50Z

cpp/src/arrow/filesystem/azurefs.cc

There is no actual directory but there could be blobs that are considered part of this implied directory. I think in this case we should delete those blobs.

I think that will require listing blobs for the prefix (internal::EnsureTrailingSlash(location.path)) then iterating through the result and deleting all those blobs.

I agree with @Tom-Newton here. Azure API might have an endpoint to delete all blobs with a certain prefix, so we don't necessarily have to loop from the client.

Ah, it makes sense. I'll do it.

Hmm. It seems that Azure Blob Storage doesn't provide an API that deletes blobs by prefix.

I'll implement it with the list and delete approach.

Tom-Newton · 2023-11-20T12:10:23Z

cpp/src/arrow/filesystem/azurefs_test.cc

Should we add a arrow::fs::AssertFileInfo(fs_.get(), path, FileType::NotFound); at the end? Personally I would probably also add an assertion that the container does exist before deleting it.

OK. I'll add them.

Tom-Newton · 2023-11-20T12:12:18Z

cpp/src/arrow/filesystem/azurefs_test.cc

I think it would be good to add a test case for a non-empty "virtual directory" where we expect the contents of the directory to be deleted.

Would be nice to add a non-empty test case for hierarchical namespace too.

I think it would be good to add a test case for a non-empty "virtual directory" where we expect the contents of the directory to be deleted.

OK. I'll add it.

Would be nice to add a non-empty test case for hierarchical namespace too.

AzureHierarchicalNamespaceFileSystemTest.DeleteDirSuccess is for the case.

felipecrv · 2023-11-20T20:50:34Z

cpp/src/arrow/filesystem/azurefs.cc

I agree with @Tom-Newton here. Azure API might have an endpoint to delete all blobs with a certain prefix, so we don't necessarily have to loop from the client.

felipecrv · 2023-11-20T22:20:08Z

cpp/src/arrow/filesystem/azurefs_test.cc

Shouldn't it delete all the blobs that start with path/to/dir/?

It make sense. I'll implement it and add a test for the case.

kou · 2023-11-21T07:40:52Z

Updated:

Add more tests.
Delete blobs under the given directory when hierarchical namespace isn't enabled (list blobs and delete them approach)

felipecrv · 2023-11-21T15:39:36Z

cpp/src/arrow/filesystem/azurefs.cc

+            return Status::IOError("Failed to delete a blob: ", blob_item.Name,
+                                   ": " + container_client.GetUrl());


I would save this on a Status variable and let all the loops continue, then check the saved status at the end.

This way we don't waste the effort made to list the blobs and a caller retrying after we return an error at the end is more likely to have less work to do.

You could cancel the outer loop listing blobs if an error was detected, but I think GetResponse should be called on all the deferred responses we have sent.

We don't need to continue the loop to avoid wasting the list effort. Because we already submitted all delete requests by SubmitBatch(). This loop just checks the results.

I think GetResponse should be called on all the deferred responses we have sent.

Why? Is it for avoiding a resource leak? I think that not calling GetResponse() doesn't leak any resource. Because I think that GetResponse() just returns a sub response returned by SubmitBatch(). I think that all sub responses are managed by SubmitBatch() response and the response is already read.
See also a SubmitBatch() response example: https://learn.microsoft.com/en-us/rest/api/storageservices/blob-batch#sample-response

Anyway, I'll check all deferred responses to show all failed blob names in error message. It'll help debug.

You're right. I thought GetResponse would block if necessary, but all the futures are waited for during SubmitBatch.

It could block, but in the specific case of batched requests, they are all at ready state ->
https://github.com/Azure/azure-sdk-for-cpp/blob/4a32d7266cfac8bfc0eb87feb56011361a36f43c/sdk/storage/azure-storage-blobs/src/blob_batch.cpp#L248

This a bit disappointing because it reduces the parallelism that could be exploited, but surely reduces the risk of API misuse.

felipecrv · 2023-11-21T15:40:16Z

cpp/src/arrow/filesystem/azurefs.cc

+      auto list_response = container_client.ListBlobs(options);
+      while (list_response.HasPage() && !list_response.Blobs.empty()) {


How are the ListBlobs errors handled here?

Oh, sorry. I missed it. I'll add try/catch for it.

cpp/src/arrow/filesystem/azurefs.cc

kou · 2023-11-22T05:10:55Z

SubmitBatch() failed only on macOS:

https://github.com/Azure/azure-sdk-for-cpp/blob/4a32d7266cfac8bfc0eb87feb56011361a36f43c/sdk/storage/azure-storage-blobs/src/blob_batch.cpp#L247-L266

'fs_->DeleteDir(directory_path)' failed with IOError: Failed to delete blobs in a directory: nvpq8dvsiy28kxtqwqa8qxqgddkka1ck: http://127.0.0.1:10000/devstoreaccount1/vai1bmd4cf5fd5s15ag8xybj1pc12z9a Azure Error: 400 One of the request inputs is not valid.

It's an Azurite bug: Azure/Azurite#2302

Can we skip the test only on macOS?

kou · 2023-11-23T14:28:19Z

Updated:

I've skipped the test only on macOS.
I've added a missing try/catch for GetResponse(). I confirmed that it may throw an exception when a subrequest is failed.

I'll merge this once CI is finished with green.

felipecrv · 2023-11-24T20:15:50Z

I've skipped the test only on macOS.

When running arrow-azurefs-test on macOS (ARM) I get an ASAN error coming from OpenSSL's libcrypto.dylib.

Nothing is wrong with the buffers passed to the function, so it must be a false positive coming from OpenSSL code.

==73846==ERROR: AddressSanitizer: container-overflow on address 0x000104d4cfa0 at pc 0x0001026ef258 bp 0x00016fdfc060 sp 0x00016fdfb820
READ of size 64 at 0x000104d4cfa0 thread T0
    #0 0x1026ef254 in wrap_memcpy+0x13c (libclang_rt.asan_osx_dynamic.dylib:arm64e+0x1b254) (BuildId: f0a7ac5c49bc3abc851181b6f92b308a32000000200000000100000000000b00)
    #1 0x100ef1908 in hmac_setkey+0x64 (libcrypto.3.dylib:arm64+0x1c9908) (BuildId: 07025f7028533fc886375706991455ee32000000200000000100000000000d00)
    #2 0x100e1ca88 in EVP_Q_mac+0x12c (libcrypto.3.dylib:arm64+0xf4a88) (BuildId: 07025f7028533fc886375706991455ee32000000200000000100000000000d00)
    #3 0x100e2a29c in HMAC+0x94 (libcrypto.3.dylib:arm64+0x10229c) (BuildId: 07025f7028533fc886375706991455ee32000000200000000100000000000d00)
    ...

kou · 2023-11-24T21:58:07Z

@felipecrv Oh... Could you open a new issue for this with full log? I'll merge this for now because our CI doesn't have ASAN on macOS. (We have it for Linux but it doesn't report this error: https://github.com/apache/arrow/actions/runs/6970987063/job/18970050943?pr=38793 )

We can work on it as a separated task.

conbench-apache-arrow · 2023-11-25T01:14:06Z

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 7da7895.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 4 possible false positives for unstable benchmarks that are known to sometimes produce them.

### Rationale for this change `DeleteDir()` deletes the given directory recursively like other filesystem implementations. ### What changes are included in this PR? * Container can be deleted with/without hierarchical namespace support. * Directory can be deleted with hierarchical namespace support. * Directory can't be deleted without hierarchical namespace support. But blobs under the given path can be deleted. So these blobs are deleted and the given virtual directory is also deleted. ### Are these changes tested? Yes. ### Are there any user-facing changes? Yes. * Closes: apache#38700 Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

github-actions bot added Component: C++ awaiting committer review Awaiting committer review labels Nov 20, 2023

Tom-Newton reviewed Nov 20, 2023

View reviewed changes

felipecrv requested changes Nov 20, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Nov 21, 2023

apacheGH-38700: [C++][FS][Azure] Implement DeleteDir()

7212e6a

kou force-pushed the cpp-azurefs-delete-dir branch from 7e25d13 to 7212e6a Compare November 21, 2023 07:31

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Nov 21, 2023

felipecrv requested changes Nov 21, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Nov 21, 2023

Add missing try/catch and support for showing all failed blob names

1bd6064

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Nov 22, 2023

kou added 2 commits November 22, 2023 12:27

Show Azurite debug log on error

e2b9beb

Add missing override

78d7132

mapleFU reviewed Nov 22, 2023

View reviewed changes

cpp/src/arrow/filesystem/azurefs.cc Show resolved Hide resolved

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Nov 22, 2023

felipecrv approved these changes Nov 22, 2023

View reviewed changes

kou added 2 commits November 23, 2023 23:21

Add missing try/catch for GetResponse()

492e754

Reduce log on error area

1dfe433

github-actions bot removed the awaiting changes Awaiting changes label Nov 23, 2023

github-actions bot added the awaiting change review Awaiting change review label Nov 23, 2023

Skip on macOS

67d3080

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Nov 23, 2023

kou mentioned this pull request Nov 23, 2023

Fix HTTP header parsing in SubmitBatch request Azure/Azurite#2302

Merged

felipecrv approved these changes Nov 23, 2023

View reviewed changes

kou merged commit 7da7895 into apache:main Nov 24, 2023

kou deleted the cpp-azurefs-delete-dir branch November 24, 2023 21:58

kou removed the awaiting changes Awaiting changes label Nov 24, 2023

		return Status::IOError("Failed to delete a blob: ", blob_item.Name,
		": " + container_client.GetUrl());

		auto list_response = container_client.ListBlobs(options);
		while (list_response.HasPage() && !list_response.Blobs.empty()) {

GH-38700: [C++][FS][Azure] Implement DeleteDir() #38793

GH-38700: [C++][FS][Azure] Implement DeleteDir() #38793

Uh oh!

Conversation

kou commented Nov 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actions bot commented Nov 20, 2023

Uh oh!

kou commented Nov 20, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kou commented Nov 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

felipecrv Nov 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kou commented Nov 22, 2023

Uh oh!

kou commented Nov 23, 2023

Uh oh!

felipecrv commented Nov 24, 2023

Uh oh!

kou commented Nov 24, 2023

Uh oh!

conbench-apache-arrow bot commented Nov 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

GH-38700: [C++][FS][Azure] Implement `DeleteDir()` #38793

GH-38700: [C++][FS][Azure] Implement `DeleteDir()` #38793

kou commented Nov 20, 2023 •

edited

Loading

kou commented Nov 21, 2023 •

edited

Loading

felipecrv Nov 21, 2023 •

edited

Loading