GH-38704: [C++] Implement Azure FileSystem Move() via Azure DataLake Storage Gen 2 API by felipecrv · Pull Request #39904 · apache/arrow

felipecrv · 2024-02-02T03:16:01Z

Rationale for this change

We need to move directories and files via the arrow::FileSystem interface.

What changes are included in this PR?

A few filesystem error reporting improvements
A helper class to deal with Azure Storage leases 1
The Move() implementation that can move files and directories within the same container on storage accounts with Hierarchical Namespace Support enabled
Lots of tests

Are these changes tested?

Yes, by existing and a huge number of tests added by this PR. The test code introduced here should be extracted to a reusable test module that we can use to test move in other file system implementations.

Are there any user-facing changes?

No breaking changes, only new functionality.

Closes: [C++][FS][Azure] Implement Move() #38704

…and blobs/files even though they are expected to behave mostly the same

There is a second use for it in later commits.

…iner()

…PI logs" This reverts commit b2134bb7752354b7d73420fb850e2e3f7b6f6baa.

kou

I took a look at all changes but I don't fully understand yet. I'll review again later.

kou · 2024-02-02T08:21:01Z

+ // "subdir0/file-at-subdir" exists
+
+ // src is a directory and dest does not exists
+ CreateDirectory(adlfs_client, "subdir0");


It seems that this is needless.

It's necessary because I'm testing the scenario where the src exists. The next line moves the subdir0 to subdir1.

It seems that subdir0 created at https://github.com/apache/arrow/pull/39904/files#diff-7d2cbf9c6abf1ee980b8cf5a79e222543c17b3d93b12c4c0c9fa123b1bb200b6R1238 still exist.

Right. Right. Removing it.

kou · 2024-02-02T08:22:00Z

+ CreateDirectory(adlfs_client, "subdir1");
+ CreateDirectory(adlfs_client, "subdir2");


It seems that they are needless.

kou · 2024-02-02T08:24:44Z

+ latest < break_or_expires_at_ &&
+ !latest_known_expiry_time_.compare_exchange_weak(latest, break_or_expires_at_)) {
+ }
+ DCHECK_GE(latest_known_expiry_time_.load(), break_or_expires_at_);


Is it safe?
I think that latest_known_expiry_time_ may be changed between latest_known_expiry_time_.compare_exchange_weak() and latest_known_expiry_time_.load().

It's safe because latest_known_expiry_time_ monotonically increases (it never goes down). So even if it's changed, the [G]reater than or [E]qual will always work.

kou · 2024-02-02T08:27:45Z

+ /// doesn't exist, otherwise a PathNotFound(location) error is produced right away
+ /// \return A BlobLeaseClient is wrapped as a unique_ptr so it's moveable and
+ /// optional (nullptr denotes blob not found)
+ Result<std::unique_ptr<Blobs::BlobLeaseClient>> AcquireBlobLease(


It seems that most codes are duplicated with AcquireContainerLease(). Can we unify them?

They use different SDK classes and the error handling is subtly different between each. Unifying these would require a lot of templating that would obfuscate the code more than clarify it.

zeroshade · 2024-02-05T05:13:07Z

+ static constexpr std::chrono::seconds kMaxLeaseDuration{60};
+
+ public:
+ LeaseGuard(std::unique_ptr<Blobs::BlobLeaseClient> lease_client,


Does it really make sense for this to be controlled by a consumer vs being controlled more internally? Is this a common pattern with Azure outside of our usage?

What you mean by "consumer" and "more internally" here? The Arrow implementation is internal compared to the software using Arrow.
Leases are a common Distributed Systems pattern [1] and the multi-step operations being performed here would have almost unpredictable outcomes in the presence of concurrent clients. Without concurrent mutators, they are very cheap (lead to no delays at all) and with concurrent mutators, they lead to outcomes we and users can reason about. Note that I often use the lease acquisition as an existence check I would have to do anyways.
[1] https://martinfowler.com/articles/patterns-of-distributed-systems/lease.html

What you mean by "consumer" and "more internally" here? The Arrow implementation is internal compared to the software using Arrow.
I'm referring to anyone using arrow::filesystem::AzureFileSystem directly, whether inside the arrow library (datasets) or not.
Leases are a common Distributed Systems pattern [1] and the multi-step operations being performed here would have almost unpredictable outcomes in the presence of concurrent clients
Yup, I know. I'm just referring to where the control of the lease is managed. But I also just realized that this entire leaseguard class isn't publicly exposed haha. Making my entire question here moot. So we're all good.

A-ha. Yes, the class is totally private. It will get more use-cases but they will all be within this file.

I might move it to a separate internal.h/cc but I would prefer doing it later to reduce noise in these PR as a lot would be moving.

felipecrv · 2024-02-05T19:30:09Z

+ //
+ // NOTE: The initial constant values were chosen conservatively. If we learn,
+ // from experience, that they are causing issues, we can increase them. And if
+ // broadly applicable values aren't possible, we can make them configurable.


@zeroshade There isn't much to these numbers, but what I can say is that they work well for a client running in Brazil talking to a storage account in a US east coast zone replicated across the US.

It might make sense to make them configurable right off the bat?

I think that would be premature. My plan if these constants are not good enough:
Raise lease time to 30s.
Raise operation times to 15s.
Network slow downs are unbounded, but failing without data loss risk and allowing a retry would be the way to go here IMO.

(I hardcoded GetUrl on the SDK class to debug my changes :)

kou

+1

kou · 2024-02-08T05:02:19Z

+ ARROW_ASSIGN_OR_RAISE(auto src_lease_client,
+ AcquireContainerLease(src, kLeaseDuration));
+ LeaseGuard src_lease_guard{std::move(src_lease_client), kLeaseDuration};


We must specify the same lease duration to AcquireContainer() and LeaseGuard::LeaseGuard(), right?
It may be misused.
Can we return std::unique_ptr<LeaseGuard> by AcquireContainerLease() to avoid creating a LeaseGuard manually?

I considered this, but the problem is that the LeaseGuard should often live in an outer scope relative to the AcquireContainerLease call, so I considered this misuse trap less bad than writing code that declares the guard far from where it's used -- I do that now sometime with optionals and I think that communicates the intent more clearly.

kou · 2024-02-08T05:04:49Z

+ try {
+ auto src_list_response = src_container_client.ListBlobs(list_blobs_options);
+ if (!src_list_response.Blobs.empty()) {
+ return Status::IOError("Unable to replace empty container: '", dest.all, "'");


Suggested change
return Status::IOError("Unable to replace empty container: '", dest.all, "'");
return Status::IOError("Unable to replace by non empty container: '", src.all, "'");

I added a comment explaining why dest is the correct here.

kou · 2024-02-08T05:07:53Z

+ }
+ try {
+ src_lease_guard.BreakBeforeDeletion(kTimeNeededForContainerDeletion);
+ src_container_client.DeleteIfExists(options);


Why do we need to use DeleteIfExists() here? Can we use Delete() here?

Yes, Delete is enough. I'm changing.

kou · 2024-02-08T05:35:00Z

+
+ // These functions are marked ARROW_NOINLINE because they are called from
+ // multiple locations, but are not performance-critical.
+


Suggested change

This comment is about the 3 functions below, not only the immediately next one.

kou · 2024-02-08T05:39:52Z

+ // TODO(felipecrv): investigate why this can't be false
+ select.allow_not_found = true;


Do you want to solve this in this PR?
If you want to defer this to a follow-up task, could you create an issue for it?

kou · 2024-02-08T05:50:12Z

+ GTEST_SKIP()
+ << "The rest of TestMovePaths is not implemented for non-HNS scenarios";


Can we just return here?
Should we use GTEST_SKIP() here? We have some tests for non-HNS case.

I would prefer to more loudly communicate that MOST of the tests are not in fact running. The return is too subtle.

conbench-apache-arrow · 2024-02-10T04:54:47Z

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

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.

…aLake Storage Gen 2 API (apache#39904) ### Rationale for this change We need to move directories and files via the `arrow::FileSystem` interface. ### What changes are included in this PR? - A few filesystem error reporting improvements - A helper class to deal with Azure Storage leases [1] - The `Move()` implementation that can move files and directories within the same container on storage accounts with Hierarchical Namespace Support enabled - Lots of tests [1]: https://learn.microsoft.com/en-us/rest/api/storageservices/lease-blob ### Are these changes tested? Yes, by existing and a huge number of tests added by this PR. The test code introduced here should be extracted to a reusable test module that we can use to test move in other file system implementations. ### Are there any user-facing changes? No breaking changes, only new functionality. * Closes: apache#38704 Authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com> Signed-off-by: Felipe Oliveira Carvalho <felipekde@gmail.com>

felipecrv · 2024-02-02T03:16:01Z

Rationale for this change

We need to move directories and files via the arrow::FileSystem interface.

What changes are included in this PR?

A few filesystem error reporting improvements
A helper class to deal with Azure Storage leases 1
The Move() implementation that can move files and directories within the same container on storage accounts with Hierarchical Namespace Support enabled
Lots of tests

Are these changes tested?

Yes, by existing and a huge number of tests added by this PR. The test code introduced here should be extracted to a reusable test module that we can use to test move in other file system implementations.

Are there any user-facing changes?

No breaking changes, only new functionality.

Closes: [C++][FS][Azure] Implement Move() #38704

…and blobs/files even though they are expected to behave mostly the same

There is a second use for it in later commits.

…iner()

…PI logs" This reverts commit b2134bb7752354b7d73420fb850e2e3f7b6f6baa.

kou

I took a look at all changes but I don't fully understand yet. I'll review again later.

kou · 2024-02-02T08:21:01Z

+ // "subdir0/file-at-subdir" exists
+
+ // src is a directory and dest does not exists
+ CreateDirectory(adlfs_client, "subdir0");


It seems that this is needless.

It's necessary because I'm testing the scenario where the src exists. The next line moves the subdir0 to subdir1.

It seems that subdir0 created at https://github.com/apache/arrow/pull/39904/files#diff-7d2cbf9c6abf1ee980b8cf5a79e222543c17b3d93b12c4c0c9fa123b1bb200b6R1238 still exist.

Right. Right. Removing it.

kou · 2024-02-02T08:22:00Z

+ CreateDirectory(adlfs_client, "subdir1");
+ CreateDirectory(adlfs_client, "subdir2");


It seems that they are needless.

kou · 2024-02-02T08:24:44Z

+ latest < break_or_expires_at_ &&
+ !latest_known_expiry_time_.compare_exchange_weak(latest, break_or_expires_at_)) {
+ }
+ DCHECK_GE(latest_known_expiry_time_.load(), break_or_expires_at_);


Is it safe?
I think that latest_known_expiry_time_ may be changed between latest_known_expiry_time_.compare_exchange_weak() and latest_known_expiry_time_.load().

It's safe because latest_known_expiry_time_ monotonically increases (it never goes down). So even if it's changed, the [G]reater than or [E]qual will always work.

kou · 2024-02-02T08:27:45Z

+ /// doesn't exist, otherwise a PathNotFound(location) error is produced right away
+ /// \return A BlobLeaseClient is wrapped as a unique_ptr so it's moveable and
+ /// optional (nullptr denotes blob not found)
+ Result<std::unique_ptr<Blobs::BlobLeaseClient>> AcquireBlobLease(


It seems that most codes are duplicated with AcquireContainerLease(). Can we unify them?

They use different SDK classes and the error handling is subtly different between each. Unifying these would require a lot of templating that would obfuscate the code more than clarify it.

zeroshade · 2024-02-05T05:13:07Z

+ static constexpr std::chrono::seconds kMaxLeaseDuration{60};
+
+ public:
+ LeaseGuard(std::unique_ptr<Blobs::BlobLeaseClient> lease_client,


Does it really make sense for this to be controlled by a consumer vs being controlled more internally? Is this a common pattern with Azure outside of our usage?

What you mean by "consumer" and "more internally" here? The Arrow implementation is internal compared to the software using Arrow.
Leases are a common Distributed Systems pattern [1] and the multi-step operations being performed here would have almost unpredictable outcomes in the presence of concurrent clients. Without concurrent mutators, they are very cheap (lead to no delays at all) and with concurrent mutators, they lead to outcomes we and users can reason about. Note that I often use the lease acquisition as an existence check I would have to do anyways.
[1] https://martinfowler.com/articles/patterns-of-distributed-systems/lease.html

What you mean by "consumer" and "more internally" here? The Arrow implementation is internal compared to the software using Arrow.
I'm referring to anyone using arrow::filesystem::AzureFileSystem directly, whether inside the arrow library (datasets) or not.
Leases are a common Distributed Systems pattern [1] and the multi-step operations being performed here would have almost unpredictable outcomes in the presence of concurrent clients
Yup, I know. I'm just referring to where the control of the lease is managed. But I also just realized that this entire leaseguard class isn't publicly exposed haha. Making my entire question here moot. So we're all good.

A-ha. Yes, the class is totally private. It will get more use-cases but they will all be within this file.

I might move it to a separate internal.h/cc but I would prefer doing it later to reduce noise in these PR as a lot would be moving.

felipecrv · 2024-02-05T19:30:09Z

+ //
+ // NOTE: The initial constant values were chosen conservatively. If we learn,
+ // from experience, that they are causing issues, we can increase them. And if
+ // broadly applicable values aren't possible, we can make them configurable.


@zeroshade There isn't much to these numbers, but what I can say is that they work well for a client running in Brazil talking to a storage account in a US east coast zone replicated across the US.

It might make sense to make them configurable right off the bat?

I think that would be premature. My plan if these constants are not good enough:
Raise lease time to 30s.
Raise operation times to 15s.
Network slow downs are unbounded, but failing without data loss risk and allowing a retry would be the way to go here IMO.

(I hardcoded GetUrl on the SDK class to debug my changes :)

kou

+1

kou · 2024-02-08T05:02:19Z

+ ARROW_ASSIGN_OR_RAISE(auto src_lease_client,
+ AcquireContainerLease(src, kLeaseDuration));
+ LeaseGuard src_lease_guard{std::move(src_lease_client), kLeaseDuration};


We must specify the same lease duration to AcquireContainer() and LeaseGuard::LeaseGuard(), right?
It may be misused.
Can we return std::unique_ptr<LeaseGuard> by AcquireContainerLease() to avoid creating a LeaseGuard manually?

I considered this, but the problem is that the LeaseGuard should often live in an outer scope relative to the AcquireContainerLease call, so I considered this misuse trap less bad than writing code that declares the guard far from where it's used -- I do that now sometime with optionals and I think that communicates the intent more clearly.

kou · 2024-02-08T05:04:49Z

+ try {
+ auto src_list_response = src_container_client.ListBlobs(list_blobs_options);
+ if (!src_list_response.Blobs.empty()) {
+ return Status::IOError("Unable to replace empty container: '", dest.all, "'");


Suggested change
return Status::IOError("Unable to replace empty container: '", dest.all, "'");
return Status::IOError("Unable to replace by non empty container: '", src.all, "'");

I added a comment explaining why dest is the correct here.

kou · 2024-02-08T05:07:53Z

+ }
+ try {
+ src_lease_guard.BreakBeforeDeletion(kTimeNeededForContainerDeletion);
+ src_container_client.DeleteIfExists(options);


Why do we need to use DeleteIfExists() here? Can we use Delete() here?

Yes, Delete is enough. I'm changing.

kou · 2024-02-08T05:35:00Z

+
+ // These functions are marked ARROW_NOINLINE because they are called from
+ // multiple locations, but are not performance-critical.
+


Suggested change

This comment is about the 3 functions below, not only the immediately next one.

kou · 2024-02-08T05:39:52Z

+ // TODO(felipecrv): investigate why this can't be false
+ select.allow_not_found = true;


Do you want to solve this in this PR?
If you want to defer this to a follow-up task, could you create an issue for it?

kou · 2024-02-08T05:50:12Z

+ GTEST_SKIP()
+ << "The rest of TestMovePaths is not implemented for non-HNS scenarios";


Can we just return here?
Should we use GTEST_SKIP() here? We have some tests for non-HNS case.

I would prefer to more loudly communicate that MOST of the tests are not in fact running. The return is too subtle.

conbench-apache-arrow · 2024-02-10T04:54:47Z

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

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.

…aLake Storage Gen 2 API (apache#39904) ### Rationale for this change We need to move directories and files via the `arrow::FileSystem` interface. ### What changes are included in this PR? - A few filesystem error reporting improvements - A helper class to deal with Azure Storage leases [1] - The `Move()` implementation that can move files and directories within the same container on storage accounts with Hierarchical Namespace Support enabled - Lots of tests [1]: https://learn.microsoft.com/en-us/rest/api/storageservices/lease-blob ### Are these changes tested? Yes, by existing and a huge number of tests added by this PR. The test code introduced here should be extracted to a reusable test module that we can use to test move in other file system implementations. ### Are there any user-facing changes? No breaking changes, only new functionality. * Closes: apache#38704 Authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com> Signed-off-by: Felipe Oliveira Carvalho <felipekde@gmail.com>

felipecrv · 2024-02-02T03:16:01Z

Rationale for this change

We need to move directories and files via the arrow::FileSystem interface.

What changes are included in this PR?

A few filesystem error reporting improvements
A helper class to deal with Azure Storage leases 1
The Move() implementation that can move files and directories within the same container on storage accounts with Hierarchical Namespace Support enabled
Lots of tests

Are these changes tested?

Yes, by existing and a huge number of tests added by this PR. The test code introduced here should be extracted to a reusable test module that we can use to test move in other file system implementations.

Are there any user-facing changes?

No breaking changes, only new functionality.

Closes: [C++][FS][Azure] Implement Move() #38704

…and blobs/files even though they are expected to behave mostly the same

There is a second use for it in later commits.

…iner()

…PI logs" This reverts commit b2134bb7752354b7d73420fb850e2e3f7b6f6baa.

kou

I took a look at all changes but I don't fully understand yet. I'll review again later.

kou · 2024-02-02T08:21:01Z

+ // "subdir0/file-at-subdir" exists
+
+ // src is a directory and dest does not exists
+ CreateDirectory(adlfs_client, "subdir0");


It seems that this is needless.

It's necessary because I'm testing the scenario where the src exists. The next line moves the subdir0 to subdir1.

It seems that subdir0 created at https://github.com/apache/arrow/pull/39904/files#diff-7d2cbf9c6abf1ee980b8cf5a79e222543c17b3d93b12c4c0c9fa123b1bb200b6R1238 still exist.

Right. Right. Removing it.

kou · 2024-02-02T08:22:00Z

+ CreateDirectory(adlfs_client, "subdir1");
+ CreateDirectory(adlfs_client, "subdir2");


It seems that they are needless.

kou · 2024-02-02T08:24:44Z

+ latest < break_or_expires_at_ &&
+ !latest_known_expiry_time_.compare_exchange_weak(latest, break_or_expires_at_)) {
+ }
+ DCHECK_GE(latest_known_expiry_time_.load(), break_or_expires_at_);


Is it safe?
I think that latest_known_expiry_time_ may be changed between latest_known_expiry_time_.compare_exchange_weak() and latest_known_expiry_time_.load().

It's safe because latest_known_expiry_time_ monotonically increases (it never goes down). So even if it's changed, the [G]reater than or [E]qual will always work.

kou · 2024-02-02T08:27:45Z

+ /// doesn't exist, otherwise a PathNotFound(location) error is produced right away
+ /// \return A BlobLeaseClient is wrapped as a unique_ptr so it's moveable and
+ /// optional (nullptr denotes blob not found)
+ Result<std::unique_ptr<Blobs::BlobLeaseClient>> AcquireBlobLease(


It seems that most codes are duplicated with AcquireContainerLease(). Can we unify them?

They use different SDK classes and the error handling is subtly different between each. Unifying these would require a lot of templating that would obfuscate the code more than clarify it.

zeroshade · 2024-02-05T05:13:07Z

+ static constexpr std::chrono::seconds kMaxLeaseDuration{60};
+
+ public:
+ LeaseGuard(std::unique_ptr<Blobs::BlobLeaseClient> lease_client,


Does it really make sense for this to be controlled by a consumer vs being controlled more internally? Is this a common pattern with Azure outside of our usage?

What you mean by "consumer" and "more internally" here? The Arrow implementation is internal compared to the software using Arrow.
Leases are a common Distributed Systems pattern [1] and the multi-step operations being performed here would have almost unpredictable outcomes in the presence of concurrent clients. Without concurrent mutators, they are very cheap (lead to no delays at all) and with concurrent mutators, they lead to outcomes we and users can reason about. Note that I often use the lease acquisition as an existence check I would have to do anyways.
[1] https://martinfowler.com/articles/patterns-of-distributed-systems/lease.html

What you mean by "consumer" and "more internally" here? The Arrow implementation is internal compared to the software using Arrow.
I'm referring to anyone using arrow::filesystem::AzureFileSystem directly, whether inside the arrow library (datasets) or not.
Leases are a common Distributed Systems pattern [1] and the multi-step operations being performed here would have almost unpredictable outcomes in the presence of concurrent clients
Yup, I know. I'm just referring to where the control of the lease is managed. But I also just realized that this entire leaseguard class isn't publicly exposed haha. Making my entire question here moot. So we're all good.

A-ha. Yes, the class is totally private. It will get more use-cases but they will all be within this file.

I might move it to a separate internal.h/cc but I would prefer doing it later to reduce noise in these PR as a lot would be moving.

felipecrv · 2024-02-05T19:30:09Z

+ //
+ // NOTE: The initial constant values were chosen conservatively. If we learn,
+ // from experience, that they are causing issues, we can increase them. And if
+ // broadly applicable values aren't possible, we can make them configurable.


@zeroshade There isn't much to these numbers, but what I can say is that they work well for a client running in Brazil talking to a storage account in a US east coast zone replicated across the US.

It might make sense to make them configurable right off the bat?

I think that would be premature. My plan if these constants are not good enough:
Raise lease time to 30s.
Raise operation times to 15s.
Network slow downs are unbounded, but failing without data loss risk and allowing a retry would be the way to go here IMO.

(I hardcoded GetUrl on the SDK class to debug my changes :)

kou

+1

kou · 2024-02-08T05:02:19Z

+ ARROW_ASSIGN_OR_RAISE(auto src_lease_client,
+ AcquireContainerLease(src, kLeaseDuration));
+ LeaseGuard src_lease_guard{std::move(src_lease_client), kLeaseDuration};


We must specify the same lease duration to AcquireContainer() and LeaseGuard::LeaseGuard(), right?
It may be misused.
Can we return std::unique_ptr<LeaseGuard> by AcquireContainerLease() to avoid creating a LeaseGuard manually?

I considered this, but the problem is that the LeaseGuard should often live in an outer scope relative to the AcquireContainerLease call, so I considered this misuse trap less bad than writing code that declares the guard far from where it's used -- I do that now sometime with optionals and I think that communicates the intent more clearly.

kou · 2024-02-08T05:04:49Z

+ try {
+ auto src_list_response = src_container_client.ListBlobs(list_blobs_options);
+ if (!src_list_response.Blobs.empty()) {
+ return Status::IOError("Unable to replace empty container: '", dest.all, "'");


Suggested change
return Status::IOError("Unable to replace empty container: '", dest.all, "'");
return Status::IOError("Unable to replace by non empty container: '", src.all, "'");

I added a comment explaining why dest is the correct here.

kou · 2024-02-08T05:07:53Z

+ }
+ try {
+ src_lease_guard.BreakBeforeDeletion(kTimeNeededForContainerDeletion);
+ src_container_client.DeleteIfExists(options);


Why do we need to use DeleteIfExists() here? Can we use Delete() here?

Yes, Delete is enough. I'm changing.

kou · 2024-02-08T05:35:00Z

+
+ // These functions are marked ARROW_NOINLINE because they are called from
+ // multiple locations, but are not performance-critical.
+


Suggested change

This comment is about the 3 functions below, not only the immediately next one.

kou · 2024-02-08T05:39:52Z

+ // TODO(felipecrv): investigate why this can't be false
+ select.allow_not_found = true;


Do you want to solve this in this PR?
If you want to defer this to a follow-up task, could you create an issue for it?

kou · 2024-02-08T05:50:12Z

+ GTEST_SKIP()
+ << "The rest of TestMovePaths is not implemented for non-HNS scenarios";


Can we just return here?
Should we use GTEST_SKIP() here? We have some tests for non-HNS case.

I would prefer to more loudly communicate that MOST of the tests are not in fact running. The return is too subtle.

conbench-apache-arrow · 2024-02-10T04:54:47Z

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

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.

…aLake Storage Gen 2 API (apache#39904) ### Rationale for this change We need to move directories and files via the `arrow::FileSystem` interface. ### What changes are included in this PR? - A few filesystem error reporting improvements - A helper class to deal with Azure Storage leases [1] - The `Move()` implementation that can move files and directories within the same container on storage accounts with Hierarchical Namespace Support enabled - Lots of tests [1]: https://learn.microsoft.com/en-us/rest/api/storageservices/lease-blob ### Are these changes tested? Yes, by existing and a huge number of tests added by this PR. The test code introduced here should be extracted to a reusable test module that we can use to test move in other file system implementations. ### Are there any user-facing changes? No breaking changes, only new functionality. * Closes: apache#38704 Authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com> Signed-off-by: Felipe Oliveira Carvalho <felipekde@gmail.com>

felipecrv · 2024-02-02T03:16:01Z

Rationale for this change

We need to move directories and files via the arrow::FileSystem interface.

What changes are included in this PR?

A few filesystem error reporting improvements
A helper class to deal with Azure Storage leases 1
The Move() implementation that can move files and directories within the same container on storage accounts with Hierarchical Namespace Support enabled
Lots of tests

Are these changes tested?

Yes, by existing and a huge number of tests added by this PR. The test code introduced here should be extracted to a reusable test module that we can use to test move in other file system implementations.

Are there any user-facing changes?

No breaking changes, only new functionality.

Closes: [C++][FS][Azure] Implement Move() #38704

…and blobs/files even though they are expected to behave mostly the same

There is a second use for it in later commits.

…iner()

…PI logs" This reverts commit b2134bb7752354b7d73420fb850e2e3f7b6f6baa.

kou

I took a look at all changes but I don't fully understand yet. I'll review again later.

kou · 2024-02-02T08:21:01Z

+ // "subdir0/file-at-subdir" exists
+
+ // src is a directory and dest does not exists
+ CreateDirectory(adlfs_client, "subdir0");


It seems that this is needless.

It's necessary because I'm testing the scenario where the src exists. The next line moves the subdir0 to subdir1.

It seems that subdir0 created at https://github.com/apache/arrow/pull/39904/files#diff-7d2cbf9c6abf1ee980b8cf5a79e222543c17b3d93b12c4c0c9fa123b1bb200b6R1238 still exist.

Right. Right. Removing it.

kou · 2024-02-02T08:22:00Z

+ CreateDirectory(adlfs_client, "subdir1");
+ CreateDirectory(adlfs_client, "subdir2");


It seems that they are needless.

kou · 2024-02-02T08:24:44Z

+ latest < break_or_expires_at_ &&
+ !latest_known_expiry_time_.compare_exchange_weak(latest, break_or_expires_at_)) {
+ }
+ DCHECK_GE(latest_known_expiry_time_.load(), break_or_expires_at_);


Is it safe?
I think that latest_known_expiry_time_ may be changed between latest_known_expiry_time_.compare_exchange_weak() and latest_known_expiry_time_.load().

It's safe because latest_known_expiry_time_ monotonically increases (it never goes down). So even if it's changed, the [G]reater than or [E]qual will always work.

kou · 2024-02-02T08:27:45Z

+ /// doesn't exist, otherwise a PathNotFound(location) error is produced right away
+ /// \return A BlobLeaseClient is wrapped as a unique_ptr so it's moveable and
+ /// optional (nullptr denotes blob not found)
+ Result<std::unique_ptr<Blobs::BlobLeaseClient>> AcquireBlobLease(


It seems that most codes are duplicated with AcquireContainerLease(). Can we unify them?

They use different SDK classes and the error handling is subtly different between each. Unifying these would require a lot of templating that would obfuscate the code more than clarify it.

zeroshade · 2024-02-05T05:13:07Z

+ static constexpr std::chrono::seconds kMaxLeaseDuration{60};
+
+ public:
+ LeaseGuard(std::unique_ptr<Blobs::BlobLeaseClient> lease_client,


Does it really make sense for this to be controlled by a consumer vs being controlled more internally? Is this a common pattern with Azure outside of our usage?

What you mean by "consumer" and "more internally" here? The Arrow implementation is internal compared to the software using Arrow.
Leases are a common Distributed Systems pattern [1] and the multi-step operations being performed here would have almost unpredictable outcomes in the presence of concurrent clients. Without concurrent mutators, they are very cheap (lead to no delays at all) and with concurrent mutators, they lead to outcomes we and users can reason about. Note that I often use the lease acquisition as an existence check I would have to do anyways.
[1] https://martinfowler.com/articles/patterns-of-distributed-systems/lease.html

What you mean by "consumer" and "more internally" here? The Arrow implementation is internal compared to the software using Arrow.
I'm referring to anyone using arrow::filesystem::AzureFileSystem directly, whether inside the arrow library (datasets) or not.
Leases are a common Distributed Systems pattern [1] and the multi-step operations being performed here would have almost unpredictable outcomes in the presence of concurrent clients
Yup, I know. I'm just referring to where the control of the lease is managed. But I also just realized that this entire leaseguard class isn't publicly exposed haha. Making my entire question here moot. So we're all good.

A-ha. Yes, the class is totally private. It will get more use-cases but they will all be within this file.

I might move it to a separate internal.h/cc but I would prefer doing it later to reduce noise in these PR as a lot would be moving.

felipecrv · 2024-02-05T19:30:09Z

+ //
+ // NOTE: The initial constant values were chosen conservatively. If we learn,
+ // from experience, that they are causing issues, we can increase them. And if
+ // broadly applicable values aren't possible, we can make them configurable.


@zeroshade There isn't much to these numbers, but what I can say is that they work well for a client running in Brazil talking to a storage account in a US east coast zone replicated across the US.

It might make sense to make them configurable right off the bat?

I think that would be premature. My plan if these constants are not good enough:
Raise lease time to 30s.
Raise operation times to 15s.
Network slow downs are unbounded, but failing without data loss risk and allowing a retry would be the way to go here IMO.

(I hardcoded GetUrl on the SDK class to debug my changes :)

kou

+1

kou · 2024-02-08T05:02:19Z

+ ARROW_ASSIGN_OR_RAISE(auto src_lease_client,
+ AcquireContainerLease(src, kLeaseDuration));
+ LeaseGuard src_lease_guard{std::move(src_lease_client), kLeaseDuration};


We must specify the same lease duration to AcquireContainer() and LeaseGuard::LeaseGuard(), right?
It may be misused.
Can we return std::unique_ptr<LeaseGuard> by AcquireContainerLease() to avoid creating a LeaseGuard manually?

I considered this, but the problem is that the LeaseGuard should often live in an outer scope relative to the AcquireContainerLease call, so I considered this misuse trap less bad than writing code that declares the guard far from where it's used -- I do that now sometime with optionals and I think that communicates the intent more clearly.

kou · 2024-02-08T05:04:49Z

+ try {
+ auto src_list_response = src_container_client.ListBlobs(list_blobs_options);
+ if (!src_list_response.Blobs.empty()) {
+ return Status::IOError("Unable to replace empty container: '", dest.all, "'");


Suggested change
return Status::IOError("Unable to replace empty container: '", dest.all, "'");
return Status::IOError("Unable to replace by non empty container: '", src.all, "'");

I added a comment explaining why dest is the correct here.

kou · 2024-02-08T05:07:53Z

+ }
+ try {
+ src_lease_guard.BreakBeforeDeletion(kTimeNeededForContainerDeletion);
+ src_container_client.DeleteIfExists(options);


Why do we need to use DeleteIfExists() here? Can we use Delete() here?

Yes, Delete is enough. I'm changing.

kou · 2024-02-08T05:35:00Z

+
+ // These functions are marked ARROW_NOINLINE because they are called from
+ // multiple locations, but are not performance-critical.
+


Suggested change

This comment is about the 3 functions below, not only the immediately next one.

kou · 2024-02-08T05:39:52Z

+ // TODO(felipecrv): investigate why this can't be false
+ select.allow_not_found = true;


Do you want to solve this in this PR?
If you want to defer this to a follow-up task, could you create an issue for it?

kou · 2024-02-08T05:50:12Z

+ GTEST_SKIP()
+ << "The rest of TestMovePaths is not implemented for non-HNS scenarios";


Can we just return here?
Should we use GTEST_SKIP() here? We have some tests for non-HNS case.

I would prefer to more loudly communicate that MOST of the tests are not in fact running. The return is too subtle.

conbench-apache-arrow · 2024-02-10T04:54:47Z

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

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.

…aLake Storage Gen 2 API (apache#39904) ### Rationale for this change We need to move directories and files via the `arrow::FileSystem` interface. ### What changes are included in this PR? - A few filesystem error reporting improvements - A helper class to deal with Azure Storage leases [1] - The `Move()` implementation that can move files and directories within the same container on storage accounts with Hierarchical Namespace Support enabled - Lots of tests [1]: https://learn.microsoft.com/en-us/rest/api/storageservices/lease-blob ### Are these changes tested? Yes, by existing and a huge number of tests added by this PR. The test code introduced here should be extracted to a reusable test module that we can use to test move in other file system implementations. ### Are there any user-facing changes? No breaking changes, only new functionality. * Closes: apache#38704 Authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com> Signed-off-by: Felipe Oliveira Carvalho <felipekde@gmail.com>

felipecrv · 2024-02-02T03:16:01Z

Rationale for this change

We need to move directories and files via the arrow::FileSystem interface.

What changes are included in this PR?

A few filesystem error reporting improvements
A helper class to deal with Azure Storage leases 1
The Move() implementation that can move files and directories within the same container on storage accounts with Hierarchical Namespace Support enabled
Lots of tests

Are these changes tested?

Yes, by existing and a huge number of tests added by this PR. The test code introduced here should be extracted to a reusable test module that we can use to test move in other file system implementations.

Are there any user-facing changes?

No breaking changes, only new functionality.

Closes: [C++][FS][Azure] Implement Move() #38704

…and blobs/files even though they are expected to behave mostly the same

There is a second use for it in later commits.

…iner()

…PI logs" This reverts commit b2134bb7752354b7d73420fb850e2e3f7b6f6baa.

kou

I took a look at all changes but I don't fully understand yet. I'll review again later.

kou · 2024-02-02T08:21:01Z

+ // "subdir0/file-at-subdir" exists
+
+ // src is a directory and dest does not exists
+ CreateDirectory(adlfs_client, "subdir0");


It seems that this is needless.

It's necessary because I'm testing the scenario where the src exists. The next line moves the subdir0 to subdir1.

It seems that subdir0 created at https://github.com/apache/arrow/pull/39904/files#diff-7d2cbf9c6abf1ee980b8cf5a79e222543c17b3d93b12c4c0c9fa123b1bb200b6R1238 still exist.

Right. Right. Removing it.

kou · 2024-02-02T08:22:00Z

+ CreateDirectory(adlfs_client, "subdir1");
+ CreateDirectory(adlfs_client, "subdir2");


It seems that they are needless.

kou · 2024-02-02T08:24:44Z

+ latest < break_or_expires_at_ &&
+ !latest_known_expiry_time_.compare_exchange_weak(latest, break_or_expires_at_)) {
+ }
+ DCHECK_GE(latest_known_expiry_time_.load(), break_or_expires_at_);


Is it safe?
I think that latest_known_expiry_time_ may be changed between latest_known_expiry_time_.compare_exchange_weak() and latest_known_expiry_time_.load().

It's safe because latest_known_expiry_time_ monotonically increases (it never goes down). So even if it's changed, the [G]reater than or [E]qual will always work.

kou · 2024-02-02T08:27:45Z

+ /// doesn't exist, otherwise a PathNotFound(location) error is produced right away
+ /// \return A BlobLeaseClient is wrapped as a unique_ptr so it's moveable and
+ /// optional (nullptr denotes blob not found)
+ Result<std::unique_ptr<Blobs::BlobLeaseClient>> AcquireBlobLease(


It seems that most codes are duplicated with AcquireContainerLease(). Can we unify them?

They use different SDK classes and the error handling is subtly different between each. Unifying these would require a lot of templating that would obfuscate the code more than clarify it.

zeroshade · 2024-02-05T05:13:07Z

+ static constexpr std::chrono::seconds kMaxLeaseDuration{60};
+
+ public:
+ LeaseGuard(std::unique_ptr<Blobs::BlobLeaseClient> lease_client,


Does it really make sense for this to be controlled by a consumer vs being controlled more internally? Is this a common pattern with Azure outside of our usage?

What you mean by "consumer" and "more internally" here? The Arrow implementation is internal compared to the software using Arrow.
Leases are a common Distributed Systems pattern [1] and the multi-step operations being performed here would have almost unpredictable outcomes in the presence of concurrent clients. Without concurrent mutators, they are very cheap (lead to no delays at all) and with concurrent mutators, they lead to outcomes we and users can reason about. Note that I often use the lease acquisition as an existence check I would have to do anyways.
[1] https://martinfowler.com/articles/patterns-of-distributed-systems/lease.html

What you mean by "consumer" and "more internally" here? The Arrow implementation is internal compared to the software using Arrow.
I'm referring to anyone using arrow::filesystem::AzureFileSystem directly, whether inside the arrow library (datasets) or not.
Leases are a common Distributed Systems pattern [1] and the multi-step operations being performed here would have almost unpredictable outcomes in the presence of concurrent clients
Yup, I know. I'm just referring to where the control of the lease is managed. But I also just realized that this entire leaseguard class isn't publicly exposed haha. Making my entire question here moot. So we're all good.

A-ha. Yes, the class is totally private. It will get more use-cases but they will all be within this file.

I might move it to a separate internal.h/cc but I would prefer doing it later to reduce noise in these PR as a lot would be moving.

felipecrv · 2024-02-05T19:30:09Z

+ //
+ // NOTE: The initial constant values were chosen conservatively. If we learn,
+ // from experience, that they are causing issues, we can increase them. And if
+ // broadly applicable values aren't possible, we can make them configurable.


@zeroshade There isn't much to these numbers, but what I can say is that they work well for a client running in Brazil talking to a storage account in a US east coast zone replicated across the US.

It might make sense to make them configurable right off the bat?

I think that would be premature. My plan if these constants are not good enough:
Raise lease time to 30s.
Raise operation times to 15s.
Network slow downs are unbounded, but failing without data loss risk and allowing a retry would be the way to go here IMO.

(I hardcoded GetUrl on the SDK class to debug my changes :)

kou

+1

kou · 2024-02-08T05:02:19Z

+ ARROW_ASSIGN_OR_RAISE(auto src_lease_client,
+ AcquireContainerLease(src, kLeaseDuration));
+ LeaseGuard src_lease_guard{std::move(src_lease_client), kLeaseDuration};


We must specify the same lease duration to AcquireContainer() and LeaseGuard::LeaseGuard(), right?
It may be misused.
Can we return std::unique_ptr<LeaseGuard> by AcquireContainerLease() to avoid creating a LeaseGuard manually?

I considered this, but the problem is that the LeaseGuard should often live in an outer scope relative to the AcquireContainerLease call, so I considered this misuse trap less bad than writing code that declares the guard far from where it's used -- I do that now sometime with optionals and I think that communicates the intent more clearly.

kou · 2024-02-08T05:04:49Z

+ try {
+ auto src_list_response = src_container_client.ListBlobs(list_blobs_options);
+ if (!src_list_response.Blobs.empty()) {
+ return Status::IOError("Unable to replace empty container: '", dest.all, "'");


Suggested change
return Status::IOError("Unable to replace empty container: '", dest.all, "'");
return Status::IOError("Unable to replace by non empty container: '", src.all, "'");

I added a comment explaining why dest is the correct here.

kou · 2024-02-08T05:07:53Z

+ }
+ try {
+ src_lease_guard.BreakBeforeDeletion(kTimeNeededForContainerDeletion);
+ src_container_client.DeleteIfExists(options);


Why do we need to use DeleteIfExists() here? Can we use Delete() here?

Yes, Delete is enough. I'm changing.

kou · 2024-02-08T05:35:00Z

+
+ // These functions are marked ARROW_NOINLINE because they are called from
+ // multiple locations, but are not performance-critical.
+


Suggested change

This comment is about the 3 functions below, not only the immediately next one.

kou · 2024-02-08T05:39:52Z

+ // TODO(felipecrv): investigate why this can't be false
+ select.allow_not_found = true;


Do you want to solve this in this PR?
If you want to defer this to a follow-up task, could you create an issue for it?

kou · 2024-02-08T05:50:12Z

+ GTEST_SKIP()
+ << "The rest of TestMovePaths is not implemented for non-HNS scenarios";


Can we just return here?
Should we use GTEST_SKIP() here? We have some tests for non-HNS case.

I would prefer to more loudly communicate that MOST of the tests are not in fact running. The return is too subtle.

conbench-apache-arrow · 2024-02-10T04:54:47Z

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

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.

…aLake Storage Gen 2 API (apache#39904) ### Rationale for this change We need to move directories and files via the `arrow::FileSystem` interface. ### What changes are included in this PR? - A few filesystem error reporting improvements - A helper class to deal with Azure Storage leases [1] - The `Move()` implementation that can move files and directories within the same container on storage accounts with Hierarchical Namespace Support enabled - Lots of tests [1]: https://learn.microsoft.com/en-us/rest/api/storageservices/lease-blob ### Are these changes tested? Yes, by existing and a huge number of tests added by this PR. The test code introduced here should be extracted to a reusable test module that we can use to test move in other file system implementations. ### Are there any user-facing changes? No breaking changes, only new functionality. * Closes: apache#38704 Authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com> Signed-off-by: Felipe Oliveira Carvalho <felipekde@gmail.com>

felipecrv · 2024-02-02T03:16:01Z

Rationale for this change

We need to move directories and files via the arrow::FileSystem interface.

What changes are included in this PR?

A few filesystem error reporting improvements
A helper class to deal with Azure Storage leases 1
The Move() implementation that can move files and directories within the same container on storage accounts with Hierarchical Namespace Support enabled
Lots of tests

Are these changes tested?

Yes, by existing and a huge number of tests added by this PR. The test code introduced here should be extracted to a reusable test module that we can use to test move in other file system implementations.

Are there any user-facing changes?

No breaking changes, only new functionality.

Closes: [C++][FS][Azure] Implement Move() #38704

…and blobs/files even though they are expected to behave mostly the same

There is a second use for it in later commits.

…iner()

…PI logs" This reverts commit b2134bb7752354b7d73420fb850e2e3f7b6f6baa.

kou

I took a look at all changes but I don't fully understand yet. I'll review again later.

kou · 2024-02-02T08:21:01Z

+ // "subdir0/file-at-subdir" exists
+
+ // src is a directory and dest does not exists
+ CreateDirectory(adlfs_client, "subdir0");


It seems that this is needless.

It's necessary because I'm testing the scenario where the src exists. The next line moves the subdir0 to subdir1.

It seems that subdir0 created at https://github.com/apache/arrow/pull/39904/files#diff-7d2cbf9c6abf1ee980b8cf5a79e222543c17b3d93b12c4c0c9fa123b1bb200b6R1238 still exist.

Right. Right. Removing it.

kou · 2024-02-02T08:22:00Z

+ CreateDirectory(adlfs_client, "subdir1");
+ CreateDirectory(adlfs_client, "subdir2");


It seems that they are needless.

kou · 2024-02-02T08:24:44Z

+ latest < break_or_expires_at_ &&
+ !latest_known_expiry_time_.compare_exchange_weak(latest, break_or_expires_at_)) {
+ }
+ DCHECK_GE(latest_known_expiry_time_.load(), break_or_expires_at_);


Is it safe?
I think that latest_known_expiry_time_ may be changed between latest_known_expiry_time_.compare_exchange_weak() and latest_known_expiry_time_.load().

It's safe because latest_known_expiry_time_ monotonically increases (it never goes down). So even if it's changed, the [G]reater than or [E]qual will always work.

kou · 2024-02-02T08:27:45Z

+ /// doesn't exist, otherwise a PathNotFound(location) error is produced right away
+ /// \return A BlobLeaseClient is wrapped as a unique_ptr so it's moveable and
+ /// optional (nullptr denotes blob not found)
+ Result<std::unique_ptr<Blobs::BlobLeaseClient>> AcquireBlobLease(


It seems that most codes are duplicated with AcquireContainerLease(). Can we unify them?

They use different SDK classes and the error handling is subtly different between each. Unifying these would require a lot of templating that would obfuscate the code more than clarify it.

zeroshade · 2024-02-05T05:13:07Z

+ static constexpr std::chrono::seconds kMaxLeaseDuration{60};
+
+ public:
+ LeaseGuard(std::unique_ptr<Blobs::BlobLeaseClient> lease_client,


Does it really make sense for this to be controlled by a consumer vs being controlled more internally? Is this a common pattern with Azure outside of our usage?

What you mean by "consumer" and "more internally" here? The Arrow implementation is internal compared to the software using Arrow.
Leases are a common Distributed Systems pattern [1] and the multi-step operations being performed here would have almost unpredictable outcomes in the presence of concurrent clients. Without concurrent mutators, they are very cheap (lead to no delays at all) and with concurrent mutators, they lead to outcomes we and users can reason about. Note that I often use the lease acquisition as an existence check I would have to do anyways.
[1] https://martinfowler.com/articles/patterns-of-distributed-systems/lease.html

What you mean by "consumer" and "more internally" here? The Arrow implementation is internal compared to the software using Arrow.
I'm referring to anyone using arrow::filesystem::AzureFileSystem directly, whether inside the arrow library (datasets) or not.
Leases are a common Distributed Systems pattern [1] and the multi-step operations being performed here would have almost unpredictable outcomes in the presence of concurrent clients
Yup, I know. I'm just referring to where the control of the lease is managed. But I also just realized that this entire leaseguard class isn't publicly exposed haha. Making my entire question here moot. So we're all good.

A-ha. Yes, the class is totally private. It will get more use-cases but they will all be within this file.

I might move it to a separate internal.h/cc but I would prefer doing it later to reduce noise in these PR as a lot would be moving.

felipecrv · 2024-02-05T19:30:09Z

+ //
+ // NOTE: The initial constant values were chosen conservatively. If we learn,
+ // from experience, that they are causing issues, we can increase them. And if
+ // broadly applicable values aren't possible, we can make them configurable.


@zeroshade There isn't much to these numbers, but what I can say is that they work well for a client running in Brazil talking to a storage account in a US east coast zone replicated across the US.

It might make sense to make them configurable right off the bat?

I think that would be premature. My plan if these constants are not good enough:
Raise lease time to 30s.
Raise operation times to 15s.
Network slow downs are unbounded, but failing without data loss risk and allowing a retry would be the way to go here IMO.

(I hardcoded GetUrl on the SDK class to debug my changes :)

kou

+1

kou · 2024-02-08T05:02:19Z

+ ARROW_ASSIGN_OR_RAISE(auto src_lease_client,
+ AcquireContainerLease(src, kLeaseDuration));
+ LeaseGuard src_lease_guard{std::move(src_lease_client), kLeaseDuration};


We must specify the same lease duration to AcquireContainer() and LeaseGuard::LeaseGuard(), right?
It may be misused.
Can we return std::unique_ptr<LeaseGuard> by AcquireContainerLease() to avoid creating a LeaseGuard manually?

I considered this, but the problem is that the LeaseGuard should often live in an outer scope relative to the AcquireContainerLease call, so I considered this misuse trap less bad than writing code that declares the guard far from where it's used -- I do that now sometime with optionals and I think that communicates the intent more clearly.

kou · 2024-02-08T05:04:49Z

+ try {
+ auto src_list_response = src_container_client.ListBlobs(list_blobs_options);
+ if (!src_list_response.Blobs.empty()) {
+ return Status::IOError("Unable to replace empty container: '", dest.all, "'");


Suggested change
return Status::IOError("Unable to replace empty container: '", dest.all, "'");
return Status::IOError("Unable to replace by non empty container: '", src.all, "'");

I added a comment explaining why dest is the correct here.

kou · 2024-02-08T05:07:53Z

+ }
+ try {
+ src_lease_guard.BreakBeforeDeletion(kTimeNeededForContainerDeletion);
+ src_container_client.DeleteIfExists(options);


Why do we need to use DeleteIfExists() here? Can we use Delete() here?

Yes, Delete is enough. I'm changing.

kou · 2024-02-08T05:35:00Z

+
+ // These functions are marked ARROW_NOINLINE because they are called from
+ // multiple locations, but are not performance-critical.
+


Suggested change

This comment is about the 3 functions below, not only the immediately next one.

kou · 2024-02-08T05:39:52Z

+ // TODO(felipecrv): investigate why this can't be false
+ select.allow_not_found = true;


Do you want to solve this in this PR?
If you want to defer this to a follow-up task, could you create an issue for it?

kou · 2024-02-08T05:50:12Z

+ GTEST_SKIP()
+ << "The rest of TestMovePaths is not implemented for non-HNS scenarios";


Can we just return here?
Should we use GTEST_SKIP() here? We have some tests for non-HNS case.

I would prefer to more loudly communicate that MOST of the tests are not in fact running. The return is too subtle.

conbench-apache-arrow · 2024-02-10T04:54:47Z

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

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.

…aLake Storage Gen 2 API (apache#39904) ### Rationale for this change We need to move directories and files via the `arrow::FileSystem` interface. ### What changes are included in this PR? - A few filesystem error reporting improvements - A helper class to deal with Azure Storage leases [1] - The `Move()` implementation that can move files and directories within the same container on storage accounts with Hierarchical Namespace Support enabled - Lots of tests [1]: https://learn.microsoft.com/en-us/rest/api/storageservices/lease-blob ### Are these changes tested? Yes, by existing and a huge number of tests added by this PR. The test code introduced here should be extracted to a reusable test module that we can use to test move in other file system implementations. ### Are there any user-facing changes? No breaking changes, only new functionality. * Closes: apache#38704 Authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com> Signed-off-by: Felipe Oliveira Carvalho <felipekde@gmail.com>

felipecrv · 2024-02-02T03:16:01Z

Rationale for this change

We need to move directories and files via the arrow::FileSystem interface.

What changes are included in this PR?

A few filesystem error reporting improvements
A helper class to deal with Azure Storage leases 1
The Move() implementation that can move files and directories within the same container on storage accounts with Hierarchical Namespace Support enabled
Lots of tests

Are these changes tested?

Yes, by existing and a huge number of tests added by this PR. The test code introduced here should be extracted to a reusable test module that we can use to test move in other file system implementations.

Are there any user-facing changes?

No breaking changes, only new functionality.

Closes: [C++][FS][Azure] Implement Move() #38704

…and blobs/files even though they are expected to behave mostly the same

There is a second use for it in later commits.

…iner()

…PI logs" This reverts commit b2134bb7752354b7d73420fb850e2e3f7b6f6baa.

kou

I took a look at all changes but I don't fully understand yet. I'll review again later.

kou · 2024-02-02T08:21:01Z

+ // "subdir0/file-at-subdir" exists
+
+ // src is a directory and dest does not exists
+ CreateDirectory(adlfs_client, "subdir0");


It seems that this is needless.

It's necessary because I'm testing the scenario where the src exists. The next line moves the subdir0 to subdir1.

It seems that subdir0 created at https://github.com/apache/arrow/pull/39904/files#diff-7d2cbf9c6abf1ee980b8cf5a79e222543c17b3d93b12c4c0c9fa123b1bb200b6R1238 still exist.

Right. Right. Removing it.

kou · 2024-02-02T08:22:00Z

+ CreateDirectory(adlfs_client, "subdir1");
+ CreateDirectory(adlfs_client, "subdir2");


It seems that they are needless.

kou · 2024-02-02T08:24:44Z

+ latest < break_or_expires_at_ &&
+ !latest_known_expiry_time_.compare_exchange_weak(latest, break_or_expires_at_)) {
+ }
+ DCHECK_GE(latest_known_expiry_time_.load(), break_or_expires_at_);


Is it safe?
I think that latest_known_expiry_time_ may be changed between latest_known_expiry_time_.compare_exchange_weak() and latest_known_expiry_time_.load().

It's safe because latest_known_expiry_time_ monotonically increases (it never goes down). So even if it's changed, the [G]reater than or [E]qual will always work.

kou · 2024-02-02T08:27:45Z

+ /// doesn't exist, otherwise a PathNotFound(location) error is produced right away
+ /// \return A BlobLeaseClient is wrapped as a unique_ptr so it's moveable and
+ /// optional (nullptr denotes blob not found)
+ Result<std::unique_ptr<Blobs::BlobLeaseClient>> AcquireBlobLease(


It seems that most codes are duplicated with AcquireContainerLease(). Can we unify them?

They use different SDK classes and the error handling is subtly different between each. Unifying these would require a lot of templating that would obfuscate the code more than clarify it.

zeroshade · 2024-02-05T05:13:07Z

+ static constexpr std::chrono::seconds kMaxLeaseDuration{60};
+
+ public:
+ LeaseGuard(std::unique_ptr<Blobs::BlobLeaseClient> lease_client,


Does it really make sense for this to be controlled by a consumer vs being controlled more internally? Is this a common pattern with Azure outside of our usage?

What you mean by "consumer" and "more internally" here? The Arrow implementation is internal compared to the software using Arrow.
Leases are a common Distributed Systems pattern [1] and the multi-step operations being performed here would have almost unpredictable outcomes in the presence of concurrent clients. Without concurrent mutators, they are very cheap (lead to no delays at all) and with concurrent mutators, they lead to outcomes we and users can reason about. Note that I often use the lease acquisition as an existence check I would have to do anyways.
[1] https://martinfowler.com/articles/patterns-of-distributed-systems/lease.html

What you mean by "consumer" and "more internally" here? The Arrow implementation is internal compared to the software using Arrow.
I'm referring to anyone using arrow::filesystem::AzureFileSystem directly, whether inside the arrow library (datasets) or not.
Leases are a common Distributed Systems pattern [1] and the multi-step operations being performed here would have almost unpredictable outcomes in the presence of concurrent clients
Yup, I know. I'm just referring to where the control of the lease is managed. But I also just realized that this entire leaseguard class isn't publicly exposed haha. Making my entire question here moot. So we're all good.

A-ha. Yes, the class is totally private. It will get more use-cases but they will all be within this file.

I might move it to a separate internal.h/cc but I would prefer doing it later to reduce noise in these PR as a lot would be moving.

felipecrv · 2024-02-05T19:30:09Z

+ //
+ // NOTE: The initial constant values were chosen conservatively. If we learn,
+ // from experience, that they are causing issues, we can increase them. And if
+ // broadly applicable values aren't possible, we can make them configurable.


@zeroshade There isn't much to these numbers, but what I can say is that they work well for a client running in Brazil talking to a storage account in a US east coast zone replicated across the US.

It might make sense to make them configurable right off the bat?

I think that would be premature. My plan if these constants are not good enough:
Raise lease time to 30s.
Raise operation times to 15s.
Network slow downs are unbounded, but failing without data loss risk and allowing a retry would be the way to go here IMO.

(I hardcoded GetUrl on the SDK class to debug my changes :)

kou

+1

kou · 2024-02-08T05:02:19Z

+ ARROW_ASSIGN_OR_RAISE(auto src_lease_client,
+ AcquireContainerLease(src, kLeaseDuration));
+ LeaseGuard src_lease_guard{std::move(src_lease_client), kLeaseDuration};


We must specify the same lease duration to AcquireContainer() and LeaseGuard::LeaseGuard(), right?
It may be misused.
Can we return std::unique_ptr<LeaseGuard> by AcquireContainerLease() to avoid creating a LeaseGuard manually?

I considered this, but the problem is that the LeaseGuard should often live in an outer scope relative to the AcquireContainerLease call, so I considered this misuse trap less bad than writing code that declares the guard far from where it's used -- I do that now sometime with optionals and I think that communicates the intent more clearly.

kou · 2024-02-08T05:04:49Z

+ try {
+ auto src_list_response = src_container_client.ListBlobs(list_blobs_options);
+ if (!src_list_response.Blobs.empty()) {
+ return Status::IOError("Unable to replace empty container: '", dest.all, "'");


Suggested change
return Status::IOError("Unable to replace empty container: '", dest.all, "'");
return Status::IOError("Unable to replace by non empty container: '", src.all, "'");

I added a comment explaining why dest is the correct here.

kou · 2024-02-08T05:07:53Z

+ }
+ try {
+ src_lease_guard.BreakBeforeDeletion(kTimeNeededForContainerDeletion);
+ src_container_client.DeleteIfExists(options);


Why do we need to use DeleteIfExists() here? Can we use Delete() here?

Yes, Delete is enough. I'm changing.

kou · 2024-02-08T05:35:00Z

+
+ // These functions are marked ARROW_NOINLINE because they are called from
+ // multiple locations, but are not performance-critical.
+


Suggested change

This comment is about the 3 functions below, not only the immediately next one.

kou · 2024-02-08T05:39:52Z

+ // TODO(felipecrv): investigate why this can't be false
+ select.allow_not_found = true;


Do you want to solve this in this PR?
If you want to defer this to a follow-up task, could you create an issue for it?

kou · 2024-02-08T05:50:12Z

+ GTEST_SKIP()
+ << "The rest of TestMovePaths is not implemented for non-HNS scenarios";


Can we just return here?
Should we use GTEST_SKIP() here? We have some tests for non-HNS case.

I would prefer to more loudly communicate that MOST of the tests are not in fact running. The return is too subtle.

conbench-apache-arrow · 2024-02-10T04:54:47Z

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

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.

…aLake Storage Gen 2 API (apache#39904) ### Rationale for this change We need to move directories and files via the `arrow::FileSystem` interface. ### What changes are included in this PR? - A few filesystem error reporting improvements - A helper class to deal with Azure Storage leases [1] - The `Move()` implementation that can move files and directories within the same container on storage accounts with Hierarchical Namespace Support enabled - Lots of tests [1]: https://learn.microsoft.com/en-us/rest/api/storageservices/lease-blob ### Are these changes tested? Yes, by existing and a huge number of tests added by this PR. The test code introduced here should be extracted to a reusable test module that we can use to test move in other file system implementations. ### Are there any user-facing changes? No breaking changes, only new functionality. * Closes: apache#38704 Authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com> Signed-off-by: Felipe Oliveira Carvalho <felipekde@gmail.com>

felipecrv · 2024-02-02T03:16:01Z

Rationale for this change

We need to move directories and files via the arrow::FileSystem interface.

What changes are included in this PR?

A few filesystem error reporting improvements
A helper class to deal with Azure Storage leases 1
The Move() implementation that can move files and directories within the same container on storage accounts with Hierarchical Namespace Support enabled
Lots of tests

Are these changes tested?

Yes, by existing and a huge number of tests added by this PR. The test code introduced here should be extracted to a reusable test module that we can use to test move in other file system implementations.

Are there any user-facing changes?

No breaking changes, only new functionality.

Closes: [C++][FS][Azure] Implement Move() #38704

…and blobs/files even though they are expected to behave mostly the same

There is a second use for it in later commits.

…iner()

…PI logs" This reverts commit b2134bb7752354b7d73420fb850e2e3f7b6f6baa.

kou

I took a look at all changes but I don't fully understand yet. I'll review again later.

kou · 2024-02-02T08:21:01Z

+ // "subdir0/file-at-subdir" exists
+
+ // src is a directory and dest does not exists
+ CreateDirectory(adlfs_client, "subdir0");


It seems that this is needless.

It's necessary because I'm testing the scenario where the src exists. The next line moves the subdir0 to subdir1.

It seems that subdir0 created at https://github.com/apache/arrow/pull/39904/files#diff-7d2cbf9c6abf1ee980b8cf5a79e222543c17b3d93b12c4c0c9fa123b1bb200b6R1238 still exist.

Right. Right. Removing it.

kou · 2024-02-02T08:22:00Z

+ CreateDirectory(adlfs_client, "subdir1");
+ CreateDirectory(adlfs_client, "subdir2");


It seems that they are needless.

kou · 2024-02-02T08:24:44Z

+ latest < break_or_expires_at_ &&
+ !latest_known_expiry_time_.compare_exchange_weak(latest, break_or_expires_at_)) {
+ }
+ DCHECK_GE(latest_known_expiry_time_.load(), break_or_expires_at_);


Is it safe?
I think that latest_known_expiry_time_ may be changed between latest_known_expiry_time_.compare_exchange_weak() and latest_known_expiry_time_.load().

It's safe because latest_known_expiry_time_ monotonically increases (it never goes down). So even if it's changed, the [G]reater than or [E]qual will always work.

kou · 2024-02-02T08:27:45Z

+ /// doesn't exist, otherwise a PathNotFound(location) error is produced right away
+ /// \return A BlobLeaseClient is wrapped as a unique_ptr so it's moveable and
+ /// optional (nullptr denotes blob not found)
+ Result<std::unique_ptr<Blobs::BlobLeaseClient>> AcquireBlobLease(


It seems that most codes are duplicated with AcquireContainerLease(). Can we unify them?

They use different SDK classes and the error handling is subtly different between each. Unifying these would require a lot of templating that would obfuscate the code more than clarify it.

zeroshade · 2024-02-05T05:13:07Z

+ static constexpr std::chrono::seconds kMaxLeaseDuration{60};
+
+ public:
+ LeaseGuard(std::unique_ptr<Blobs::BlobLeaseClient> lease_client,


Does it really make sense for this to be controlled by a consumer vs being controlled more internally? Is this a common pattern with Azure outside of our usage?

What you mean by "consumer" and "more internally" here? The Arrow implementation is internal compared to the software using Arrow.
Leases are a common Distributed Systems pattern [1] and the multi-step operations being performed here would have almost unpredictable outcomes in the presence of concurrent clients. Without concurrent mutators, they are very cheap (lead to no delays at all) and with concurrent mutators, they lead to outcomes we and users can reason about. Note that I often use the lease acquisition as an existence check I would have to do anyways.
[1] https://martinfowler.com/articles/patterns-of-distributed-systems/lease.html

What you mean by "consumer" and "more internally" here? The Arrow implementation is internal compared to the software using Arrow.
I'm referring to anyone using arrow::filesystem::AzureFileSystem directly, whether inside the arrow library (datasets) or not.
Leases are a common Distributed Systems pattern [1] and the multi-step operations being performed here would have almost unpredictable outcomes in the presence of concurrent clients
Yup, I know. I'm just referring to where the control of the lease is managed. But I also just realized that this entire leaseguard class isn't publicly exposed haha. Making my entire question here moot. So we're all good.

A-ha. Yes, the class is totally private. It will get more use-cases but they will all be within this file.

I might move it to a separate internal.h/cc but I would prefer doing it later to reduce noise in these PR as a lot would be moving.

felipecrv · 2024-02-05T19:30:09Z

+ //
+ // NOTE: The initial constant values were chosen conservatively. If we learn,
+ // from experience, that they are causing issues, we can increase them. And if
+ // broadly applicable values aren't possible, we can make them configurable.


@zeroshade There isn't much to these numbers, but what I can say is that they work well for a client running in Brazil talking to a storage account in a US east coast zone replicated across the US.

It might make sense to make them configurable right off the bat?

I think that would be premature. My plan if these constants are not good enough:
Raise lease time to 30s.
Raise operation times to 15s.
Network slow downs are unbounded, but failing without data loss risk and allowing a retry would be the way to go here IMO.

(I hardcoded GetUrl on the SDK class to debug my changes :)

kou

+1

kou · 2024-02-08T05:02:19Z

+ ARROW_ASSIGN_OR_RAISE(auto src_lease_client,
+ AcquireContainerLease(src, kLeaseDuration));
+ LeaseGuard src_lease_guard{std::move(src_lease_client), kLeaseDuration};


We must specify the same lease duration to AcquireContainer() and LeaseGuard::LeaseGuard(), right?
It may be misused.
Can we return std::unique_ptr<LeaseGuard> by AcquireContainerLease() to avoid creating a LeaseGuard manually?

I considered this, but the problem is that the LeaseGuard should often live in an outer scope relative to the AcquireContainerLease call, so I considered this misuse trap less bad than writing code that declares the guard far from where it's used -- I do that now sometime with optionals and I think that communicates the intent more clearly.

kou · 2024-02-08T05:04:49Z

+ try {
+ auto src_list_response = src_container_client.ListBlobs(list_blobs_options);
+ if (!src_list_response.Blobs.empty()) {
+ return Status::IOError("Unable to replace empty container: '", dest.all, "'");


Suggested change
return Status::IOError("Unable to replace empty container: '", dest.all, "'");
return Status::IOError("Unable to replace by non empty container: '", src.all, "'");

I added a comment explaining why dest is the correct here.

kou · 2024-02-08T05:07:53Z

+ }
+ try {
+ src_lease_guard.BreakBeforeDeletion(kTimeNeededForContainerDeletion);
+ src_container_client.DeleteIfExists(options);


Why do we need to use DeleteIfExists() here? Can we use Delete() here?

Yes, Delete is enough. I'm changing.

kou · 2024-02-08T05:35:00Z

+
+ // These functions are marked ARROW_NOINLINE because they are called from
+ // multiple locations, but are not performance-critical.
+


Suggested change

This comment is about the 3 functions below, not only the immediately next one.

kou · 2024-02-08T05:39:52Z

+ // TODO(felipecrv): investigate why this can't be false
+ select.allow_not_found = true;


Do you want to solve this in this PR?
If you want to defer this to a follow-up task, could you create an issue for it?

kou · 2024-02-08T05:50:12Z

+ GTEST_SKIP()
+ << "The rest of TestMovePaths is not implemented for non-HNS scenarios";


Can we just return here?
Should we use GTEST_SKIP() here? We have some tests for non-HNS case.

I would prefer to more loudly communicate that MOST of the tests are not in fact running. The return is too subtle.

conbench-apache-arrow · 2024-02-10T04:54:47Z

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

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.

…aLake Storage Gen 2 API (apache#39904) ### Rationale for this change We need to move directories and files via the `arrow::FileSystem` interface. ### What changes are included in this PR? - A few filesystem error reporting improvements - A helper class to deal with Azure Storage leases [1] - The `Move()` implementation that can move files and directories within the same container on storage accounts with Hierarchical Namespace Support enabled - Lots of tests [1]: https://learn.microsoft.com/en-us/rest/api/storageservices/lease-blob ### Are these changes tested? Yes, by existing and a huge number of tests added by this PR. The test code introduced here should be extracted to a reusable test module that we can use to test move in other file system implementations. ### Are there any user-facing changes? No breaking changes, only new functionality. * Closes: apache#38704 Authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com> Signed-off-by: Felipe Oliveira Carvalho <felipekde@gmail.com>

felipecrv added 18 commits January 31, 2024 16:28

azurefs_test.cc: Use pcg for random number generation
c16a9c1

azurefs_test.cc: Use different APIs to create containers/filesystems …
f001b40
…and blobs/files even though they are expected to behave mostly the same

azurefs_test.cc: Provide a shorter method name for path creation
2b3067c

Add exception.ErrorCode to the Status message
9af370c

Add GetBlobClient() helper function
a0b4b6f

Extract GetFileInfoOfPathWithinContainer()
016bb39
There is a second use for it in later commits.

Add AcquireBlobLease() and AcquireContainerLease()
94ee77f

Add lease_id param to GetFileInfo(adlfs_client, ...)
8ea6b85

Add optional lease_id parameter to DeleteDirOnFileSystem
86d88c2

Introduce the LeaseGuard class
22eded5

Add implementation skeleton of FileSystem::Move() + Impl::RenameConta…
80414f1
…iner()

Work around Azurite limitations and SDK/service bugs
723d6af

Add checks for single and multi-container move operations
03cd298

Implement CreateContainerFromPath (one of the Move() scenarios)
cd238ec

Improve utilities for creating and asserting POSIX errors
a66e85a

Implement MovePaths and a huge sets of tests for it
35cbd4c

Add lease acquisition/release logs and MovePathsWithDataLakeAPI logs
b41356e

Revert "Add lease acquisition/release logs and MovePathsWithDataLakeA…
c19c574
…PI logs" This reverts commit b2134bb7752354b7d73420fb850e2e3f7b6f6baa.

felipecrv requested a review from kou February 2, 2024 03:16

github-actionsBot added Component: C++ awaiting review Awaiting review labels Feb 2, 2024

kou reviewed Feb 2, 2024
View reviewed changes

github-actionsBot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Feb 2, 2024

Incorporate early feedback by kou
005c5c4

github-actionsBot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels Feb 2, 2024

zeroshade reviewed Feb 5, 2024
View reviewed changes

github-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Feb 5, 2024

felipecrv commented Feb 5, 2024
View reviewed changes

felipecrv requested a review from zeroshade February 5, 2024 19:30

felipecrv added 2 commits February 5, 2024 16:39

io_util.h: Add missing include
0fb549b

Remove calls to a method that doesn't exist
86c551a
(I hardcoded GetUrl on the SDK class to debug my changes :)

github-actionsBot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Feb 7, 2024

kou approved these changes Feb 8, 2024
View reviewed changes

github-actionsBot added awaiting merge Awaiting merge and removed awaiting changes Awaiting changes labels Feb 8, 2024

felipecrv mentioned this pull request Feb 9, 2024
[C++][FS] AzureFileSystem::GetFileInfo(FileSelection) doesn't always handled allow_not_found correctly #40014
Open

felipecrv force-pushed the azure_move branch from 6709d61 to a035148Compare February 9, 2024 03:07

github-actionsBot added awaiting changes Awaiting changes and removed awaiting merge Awaiting merge labels Feb 9, 2024

Changes based on latest comments by kou
280a61c

felipecrv force-pushed the azure_move branch from 9f34d0f to a8622a8Compare February 9, 2024 23:17

github-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Feb 9, 2024

felipecrv added 2 commits February 9, 2024 20:39

Fix ASAN violation in NormalizerKeyValueMetadata (unrelated)
419d7df

Merge branch 'main' into azure_move
89a1ce2

Tom-Newton mentioned this pull request Feb 9, 2024
[Python][FS][Azure] Minimal python bindings for AzureFilesystem#39968
Closed

felipecrv force-pushed the azure_move branch from a8622a8 to 89a1ce2Compare February 9, 2024 23:50

felipecrv merged commit 0ce54b6 into apache:mainFeb 10, 2024

felipecrv removed the awaiting change review Awaiting change review label Feb 10, 2024

felipecrv deleted the azure_move branch February 10, 2024 02:14