Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[C++][FS][Azure] Remove StatusFromErrorResponse as it's not necessary #39718

Closed
felipecrv opened this issue Jan 20, 2024 · 0 comments · Fixed by #39719
Closed

[C++][FS][Azure] Remove StatusFromErrorResponse as it's not necessary #39718

felipecrv opened this issue Jan 20, 2024 · 0 comments · Fixed by #39719

Comments

@felipecrv
Copy link
Contributor

Describe the enhancement requested

Only the "*IfExists" functions from the Azure SDK ever set response.Value.Deleted to false to indicate that a resource wasn't found and the request succeeded without deleting anything.

This is the implementation of DataLakePathClient::DeleteIfExists:

  Azure::Response<Models::DeletePathResult> DataLakePathClient::DeleteIfExists(
      const DeletePathOptions& options,
      const Azure::Core::Context& context) const
  {
    try
    {
      return Delete(options, context);
    }
    catch (StorageException& e)
    {
      if (e.ErrorCode == _detail::DataLakeFilesystemNotFound
          || e.ErrorCode == _detail::DataLakePathNotFound)
      {
        Models::DeletePathResult ret;
        ret.Deleted = false;
        return Azure::Response<Models::DeletePathResult>(std::move(ret), std::move(e.RawResponse));
      }
      throw;
    }
  }

It's better that we use the Delete() versions of these functions instead of DeleteIfExists and check the ErrorCode ourselves to return an appropriate Status instead of something generic.

Component(s)

C++

@felipecrv felipecrv self-assigned this Jan 20, 2024
felipecrv added a commit that referenced this issue Jan 22, 2024
… necessary (#39719)

### Rationale for this change

Only the "*IfExists" functions from the Azure SDK ever set `response.Value.Deleted` to `false` to indicate that a resource wasn't found and the request succeeded without deleting anything. 

It's better that we use the `Delete()` versions of these functions instead of `DeleteIfExists` and check the `ErrorCode` ourselves to return an appropriate `Status` instead of something generic.

### What changes are included in this PR?

 - Removing `StatusFromErrorResponse`
 - Comments explaining the error handling decisions
 - Addition of a boolean parameter to `DeleteDirOnFileSystem` that controls how it fails when the directory being deleted doesn't exist
 
### Are these changes tested?

 - Yes, by the existing tests in `azurefs_test.cc`.
* Closes: #39718

Authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com>
Signed-off-by: Felipe Oliveira Carvalho <felipekde@gmail.com>
@felipecrv felipecrv added this to the 16.0.0 milestone Jan 22, 2024
dgreiss pushed a commit to dgreiss/arrow that referenced this issue Feb 19, 2024
…'s not necessary (apache#39719)

### Rationale for this change

Only the "*IfExists" functions from the Azure SDK ever set `response.Value.Deleted` to `false` to indicate that a resource wasn't found and the request succeeded without deleting anything. 

It's better that we use the `Delete()` versions of these functions instead of `DeleteIfExists` and check the `ErrorCode` ourselves to return an appropriate `Status` instead of something generic.

### What changes are included in this PR?

 - Removing `StatusFromErrorResponse`
 - Comments explaining the error handling decisions
 - Addition of a boolean parameter to `DeleteDirOnFileSystem` that controls how it fails when the directory being deleted doesn't exist
 
### Are these changes tested?

 - Yes, by the existing tests in `azurefs_test.cc`.
* Closes: apache#39718

Authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com>
Signed-off-by: Felipe Oliveira Carvalho <felipekde@gmail.com>
zanmato1984 pushed a commit to zanmato1984/arrow that referenced this issue Feb 28, 2024
…'s not necessary (apache#39719)

### Rationale for this change

Only the "*IfExists" functions from the Azure SDK ever set `response.Value.Deleted` to `false` to indicate that a resource wasn't found and the request succeeded without deleting anything. 

It's better that we use the `Delete()` versions of these functions instead of `DeleteIfExists` and check the `ErrorCode` ourselves to return an appropriate `Status` instead of something generic.

### What changes are included in this PR?

 - Removing `StatusFromErrorResponse`
 - Comments explaining the error handling decisions
 - Addition of a boolean parameter to `DeleteDirOnFileSystem` that controls how it fails when the directory being deleted doesn't exist
 
### Are these changes tested?

 - Yes, by the existing tests in `azurefs_test.cc`.
* Closes: apache#39718

Authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com>
Signed-off-by: Felipe Oliveira Carvalho <felipekde@gmail.com>
thisisnic pushed a commit to thisisnic/arrow that referenced this issue Mar 8, 2024
…'s not necessary (apache#39719)

### Rationale for this change

Only the "*IfExists" functions from the Azure SDK ever set `response.Value.Deleted` to `false` to indicate that a resource wasn't found and the request succeeded without deleting anything. 

It's better that we use the `Delete()` versions of these functions instead of `DeleteIfExists` and check the `ErrorCode` ourselves to return an appropriate `Status` instead of something generic.

### What changes are included in this PR?

 - Removing `StatusFromErrorResponse`
 - Comments explaining the error handling decisions
 - Addition of a boolean parameter to `DeleteDirOnFileSystem` that controls how it fails when the directory being deleted doesn't exist
 
### Are these changes tested?

 - Yes, by the existing tests in `azurefs_test.cc`.
* Closes: apache#39718

Authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com>
Signed-off-by: Felipe Oliveira Carvalho <felipekde@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant