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

Use specific exception type for certain kinds of storage failures #447

Closed
qc00 opened this issue May 31, 2023 · 1 comment · Fixed by #1414, #1411, #1360, #1344 or #1304
Closed

Use specific exception type for certain kinds of storage failures #447

qc00 opened this issue May 31, 2023 · 1 comment · Fixed by #1414, #1411, #1360, #1344 or #1304
Assignees
@muhammadhamzasajjad
Labels
enhancement New feature or request

Comments

@qc00
Copy link
Contributor

qc00 commented May 31, 2023

Per this comment, we should make the storage throw our event-specific exceptions (e.g. Key/symbol/segment not found) instead of generic or storage framework's exception types.

We can then tighten the exception handling in the downstream code.

Related to #368, #365
@qc00 qc00 added the enhancement New feature or request label May 31, 2023
@qc00 qc00 mentioned this issue May 31, 2023
Merged
@qc00 qc00 mentioned this issue Jun 26, 2023
Closed
@qc00
Copy link
Contributor Author

qc00 commented Jun 27, 2023 •
edited
Loading

The less inconsistent sets of exceptions thrown after my PR is merged:

Legend:

  • Lowercase letters = exception is thrown immediately on such error
  • Upper case: a list of affected keys is accumulated and exception is thrown at the end of the operation (if no other exception occurred)
  • >=a wrapper function catches the inner exception and rethrows as
  • d=DuplicateKeyException
  • k=KeyNotFoundException
  • p=PermissionException/PermissionSpecificException
  • u=unexpected exception (store-specific)
  • r=retryable exception (store-specific)
Store\Op write update read exists remove iterate
LMDB ud ku d K - K -
Mongo u kur ku^ u kur>K -
S3 u rp  urp Kurp urp K urp
Memory  d k k - k -

^ mongo wanted to do ">K" but failed to catch the inner exceptions correctly…

@qc00 qc00 mentioned this issue Jun 29, 2023
Merged
3 tasks
@phoebusm phoebusm mentioned this issue Jun 29, 2023
Closed
qc00 added a commit that referenced this issue Jul 3, 2023
@qc00
#447 Consistently use KeyNotFoundException and DuplicateKeyException
7f7207c 
Also provide meaningful messages for mongo exceptions
qc00 added a commit that referenced this issue Jul 3, 2023
@qc00
#447 Use PermissionException in more places
58d0335
@qc00 qc00 mentioned this issue Jul 4, 2023
Closed
qc00 added a commit that referenced this issue Jul 12, 2023
@qc00
#447 Consistently use KeyNotFoundException and DuplicateKeyException
4707a30 
Also provide meaningful messages for mongo exceptions
qc00 added a commit that referenced this issue Jul 12, 2023
@qc00
#447 Use PermissionException in more places
7444857
@poodlewars poodlewars added this to the Release 2 milestone Jul 31, 2023
@mehertz mehertz modified the milestones: 2.0.0, Release 3 Aug 14, 2023
@mehertz mehertz modified the milestones: Release 3, Release 4 Aug 25, 2023
@poodlewars poodlewars modified the milestones: Release 4, 4.2.0 Oct 24, 2023
@qc00 qc00 mentioned this issue Oct 30, 2023
Closed
muhammadhamzasajjad added a commit that referenced this issue Feb 2, 2024
@muhammadhamzasajjad
#447 LMDB Exceptions Normalization (#1285)
6d246d3 
Implementation of lmdb storage specific exceptions. This PR is a
refactor of #584 which was not merged because it was too complex.

This PR also implements a framework for testing storage exceptions.
`GenericStorageTest` class allows testing of common exceptions which are
triggered among all the storages. Some examples of these exceptions are
`KeyNotFoundException` if you read an in-existent key,
`DuplicateKeyException` if you attempt to overwrite a key that already
exists.

`lmdb::map_full_error` is an example of exception specific to LMDB which
is triggered when the remaining map size is not enough to write new
keys/values.
muhammadhamzasajjad added a commit that referenced this issue Feb 8, 2024
@muhammadhamzasajjad
#447 Memory Storage Exception Normalization (#1297)
da288af 
Implementation of memory storage specific exceptions normalization. This
PR is a refactor of #584.
muhammadhamzasajjad added a commit that referenced this issue Feb 13, 2024
@muhammadhamzasajjad
#447 S3 Storage Exceptions Handling (#1304)
92e6877 
This is a refactor of #584 for S3 Storage Exceptions. This PR also
implements the tests for S3 storage exceptions. Note that in some cases
S3 Storage Exceptions are different from other storages. For instance if
you write a path that already exists, other storages throw
`DuplicateKeyException`. In this case, S3 just over-writes the key
without any errors, therefore no Exception is thrown.

The function `handle_s3_error` handles other S3 Storage specific
exceptions as follows:
- `E_PERMISSION`: raised when `ACCESS_DENIED`, `INVALID_ACCESS_KEY_ID`
or `SIGNATURE_DOES_NOT_MATCH` errors are returned.
- `E_S3_RETRYABLE`: raised for all the other S3 exceptions that are
retry-able.
- `E_UNEXPECTED_S3_ERROR`: raised for all the other S3 exceptions that
are not retry-able.

These have been implemented following the PR #584.

#### 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>
muhammadhamzasajjad added a commit that referenced this issue Feb 26, 2024
@muhammadhamzasajjad
#447 Azure Storage Exceptions Normalization (#1344)
4d20f00 
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](https://learn.microsoft.com/en-us/rest/api/storageservices/blob-service-error-codes).
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.
muhammadhamzasajjad added a commit that referenced this issue Feb 28, 2024
@muhammadhamzasajjad
#447 Exception normalization for RocksDB (#1360)
8db4515 
This performs normalization of RocksDB storage exceptions as per
#584.
muhammadhamzasajjad added a commit that referenced this issue Mar 8, 2024
@muhammadhamzasajjad
#447 Add a MockMongoClient which can simulate mongo failures (#1395)
3aac66c 
Similar to #1331. The mock client will then be used to test exception
normalization. Wrap up common things like `StorageOperation` and `StorageFailure` into `storage_mock_client.hpp`
muhammadhamzasajjad added a commit that referenced this issue Mar 11, 2024
@muhammadhamzasajjad
#447 Mongo Exceptions Normalization (#1411)
3af7f61 
This normalizes mongo exceptions as in #584. The logic for exception
normalization is now moved to `mongo_storage.cpp` as we also support a
`MockMongoClient` which can mock failures. The Permission exceptions are
also normalized. Previously there were two types of permission
exceptions. One was triggered when a storage API returned a permission
failure, the second was triggered if an unpermitted storage operation
was attempted as per the library `OpenMode`. The two permission have now
been normalized to inherit from the same class.

Furthermore, the exception hierarchy in python has also been changed.
`DuplicateKeyException` inherits from `StorageException` in C++, this
should be reflected on python side too. Why? because if someone wanted
they could catch all the storage exceptions as follows instead of having
to catch `PermissionException`, `DuplicateKeyException` and
`StorageException` separately:

```python
try:
  # an operation that calls storage from python
except StorageException:
  # log storage error
```

#### 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
-->
This was linked to pull requests Mar 12, 2024
LMDB exception normalization with mock client #1414
Merged
#447 Mongo Exceptions Normalization #1411
Merged
#447 Exception normalization for RocksDB #1360
Merged
#447 Azure Storage Exceptions Normalization #1344
Merged
#447 S3 Storage Exceptions Handling #1304
Merged
#447 Memory Storage Exception Normalization #1297
Merged
#447 LMDB Exceptions Normalization #1285
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@muhammadhamzasajjad muhammadhamzasajjad

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
4 participants
@qc00 @mehertz @poodlewars @muhammadhamzasajjad