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.

Sign up for GitHub

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

Jump to bottom

#447 Azure Storage Exceptions Normalization #1344

Merged
merged 12 commits into from
Feb 26, 2024
Merged

#447 Azure Storage Exceptions Normalization #1344

merged 12 commits into from
Feb 26, 2024

Conversation

muhammadhamzasajjad
Copy link
Collaborator

This PR performs normalization of Azure storage exceptions as per #584. It throws exceptions using error code and http status code as per the azure error docs. It also does a refactor of the test_storage_exceptions.cpp using functions from common.hpp. PermissionException is renamed as StoragePermissionException as it was conflicting with another exception.

Checklist

Checklist for code changes...
  • Have you updated the relevant docstrings, documentation and copyright notice?
  • Is this contribution tested against all ArcticDB's features?
  • Do all exceptions introduced raise appropriate error messages?
  • Are API changes highlighted in the PR description?
  • Is the PR labelled as enhancement or bug so it appears in autogenerated release notes?

@muhammadhamzasajjad muhammadhamzasajjad marked this pull request as ready for review February 20, 2024 15:21
poodlewars
poodlewars approved these changes Feb 20, 2024
View reviewed changes
Copy link
Collaborator

@poodlewars poodlewars left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks excellent! Just left some minor comments. Please also remember to squash the commits before merging.

cpp/arcticdb/storage/azure/azure_client_wrapper.hpp Outdated Show resolved Hide resolved
cpp/arcticdb/storage/azure/azure_storage.cpp Outdated Show resolved Hide resolved
cpp/arcticdb/storage/azure/azure_storage.cpp Outdated Show resolved Hide resolved
cpp/arcticdb/storage/test/test_storage_exceptions.cpp Outdated Show resolved Hide resolved
phoebusm
phoebusm reviewed Feb 20, 2024
View reviewed changes
cpp/arcticdb/storage/azure/azure_storage.cpp Outdated

if(status_code == Azure::Core::Http::HttpStatusCode::Unauthorized || status_code == Azure::Core::Http::HttpStatusCode::Forbidden) {
raise<ErrorCode::E_PERMISSION>(fmt::format("Permission error: AzureError#{} {}: {}",
int(status_code), error_code, e.ReasonPhrase));
Copy link
Collaborator

@phoebusm phoebusm Feb 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's something minor, but I am feeling a bit uneasy of using (int) instead of the C++ style static_cast. (int) is effectively reinterpret_cast which offers less checking than the static_cast.

Copy link
Collaborator Author

@muhammadhamzasajjad muhammadhamzasajjad Feb 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in e3462da

@phoebusm
Copy link
Collaborator

Have the changes been test against the real azure storage?
Sorry I don't have time yet to setup a real azure storage in the pipeline. It's one me

@muhammadhamzasajjad
Copy link
Collaborator Author

Have the changes been test against the real azure storage? Sorry I don't have time yet to setup a real azure storage in the pipeline. It's one me

I did some testing on real azure storage from test_azure_storage.cpp with real azure storage setup. I also ran some of the tests from python/tests/integration/ and python/tests/unit using real azure storage instead of azurite. Note that this PR does not change any of the existing logic for read, write or other operations. The tests setup for a real azure storage is yet to be written.

@phoebusm
Copy link
Collaborator

phoebusm commented Feb 21, 2024 •
edited
Loading

@muhammadhamzasajjad I mean testing the error response/status code from the real storage. I have quite little faith in Azurite so it will be great to make sure the responses from real storage and Azurite align.
If the test looks good, the PR LGTM then.

phoebusm
phoebusm reviewed Feb 21, 2024
View reviewed changes
cpp/arcticdb/storage/azure/azure_client_wrapper.hpp
case AzureErrorCode::UnauthorizedBlobOverwrite: return "UnauthorizedBlobOverwrite";
case AzureErrorCode::InvalidBlobOrBlock: return "InvalidBlobOrBlock";
case AzureErrorCode::OtherError: return "Other Unspecified error";
}

return "Other Unspecified error";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could add this to switch default case.

@muhammadhamzasajjad muhammadhamzasajjad force-pushed the azure_storage_exceptions branch 2 times, most recently from 4ea3442 to e3462da Compare February 21, 2024 13:07
@phoebusm phoebusm added the enhancement New feature or request label Feb 21, 2024
@muhammadhamzasajjad muhammadhamzasajjad self-assigned this Feb 21, 2024
@muhammadhamzasajjad muhammadhamzasajjad merged commit 4d20f00 into master Feb 26, 2024
110 checks passed
@muhammadhamzasajjad muhammadhamzasajjad deleted the azure_storage_exceptions branch February 26, 2024 14:32
@muhammadhamzasajjad muhammadhamzasajjad mentioned this pull request Mar 12, 2024
Merged
5 tasks
@muhammadhamzasajjad muhammadhamzasajjad linked an issue Mar 12, 2024 that may be closed by this pull request
Use specific exception type for certain kinds of storage failures #447
Closed
muhammadhamzasajjad added a commit that referenced this pull request Mar 14, 2024
@muhammadhamzasajjad
LMDB exception normalization with mock client (#1414)
882e73e 
This is the final PR for exception normalization. Previous PRs are
#1411, #1360, #1344, #1304, #1297 and #1285
#### Reference Issues/PRs
Previously #1285 only normalized `KeyNotFoundException` and
`DuplicateKeyException` correctly. As mentioned in [this
comment](#1285 (comment)),
we need to normalize other lmdb specific errors too. This PR does that.
A mock client is also created to simulate the lmdb errors which aren't
easily produce-able with real lmdb but can occur.

The ErrorCode list has been updated in `error_code.hpp`. Previously, all
the storage error codes were just sequential. This required that each
time a new error was added for a specific storage, all the other error
codes needed to be changed as we want to assign sequential error codes
to the same storage. We now leave 10 error codes for each storage which
allows us to easily add new error codes for a storage without having to
change all the others.

`::lmdb::map_full_error` is now normalized. When this error occurs, lmdb
throws `LMDBMapFullException` which is child of `StorageException`

#### What does this implement or fix?

#### Any other comments?

#### Checklist

<details>
  <summary>
   Checklist for code changes...
  </summary>
 
- [ ] Have you updated the relevant docstrings, documentation and
copyright notice?
- [ ] Is this contribution tested against [all ArcticDB's
features](../docs/mkdocs/docs/technical/contributing.md)?
- [ ] Do all exceptions introduced raise appropriate [error
messages](https://docs.arcticdb.io/error_messages/)?
 - [ ] Are API changes highlighted in the PR description?
- [ ] Is the PR labelled as enhancement or bug so it appears in
autogenerated release notes?
</details>

<!--
Thanks for contributing a Pull Request to ArcticDB! Please ensure you
have taken a look at:
- ArcticDB's Code of Conduct:
https://github.com/man-group/ArcticDB/blob/master/CODE_OF_CONDUCT.md
- ArcticDB's Contribution Licensing:
https://github.com/man-group/ArcticDB/blob/master/docs/mkdocs/docs/technical/contributing.md#contribution-licensing
-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phoebusm phoebusm phoebusm left review comments

@poodlewars poodlewars poodlewars approved these changes

Assignees

@muhammadhamzasajjad muhammadhamzasajjad

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use specific exception type for certain kinds of storage failures
3 participants
@muhammadhamzasajjad @phoebusm @poodlewars