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 Mongo Exceptions Normalization #1411

Merged
merged 5 commits into from
Mar 11, 2024
Merged

#447 Mongo Exceptions Normalization #1411

merged 5 commits into from
Mar 11, 2024

Conversation

muhammadhamzasajjad
Copy link
Collaborator

@muhammadhamzasajjad muhammadhamzasajjad commented Mar 8, 2024 •
edited
Loading

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:

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

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 force-pushed the mongo_storage_exceptions branch from 52f23d4 to fef3226 Compare March 9, 2024 13:49
@muhammadhamzasajjad muhammadhamzasajjad self-assigned this Mar 11, 2024
@muhammadhamzasajjad muhammadhamzasajjad marked this pull request as ready for review March 11, 2024 12:43
@poodlewars poodlewars added the enhancement New feature or request label Mar 11, 2024
poodlewars
poodlewars requested changes Mar 11, 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 great! Just minor comments. Please squash commits before merging.

cpp/arcticdb/python/python_module.cpp Outdated Show resolved Hide resolved
cpp/arcticdb/storage/python_bindings.cpp Outdated Show resolved Hide resolved
cpp/arcticdb/storage/test/test_storage_exceptions.cpp Outdated Show resolved Hide resolved
poodlewars
poodlewars approved these changes Mar 11, 2024
View reviewed changes
Copy link
Collaborator

@poodlewars poodlewars left a comment

Choose a reason for hiding this comment

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

Looks good! Please squash before you merge this in

@muhammadhamzasajjad muhammadhamzasajjad merged commit 3af7f61 into master Mar 11, 2024
114 checks passed
@muhammadhamzasajjad muhammadhamzasajjad deleted the mongo_storage_exceptions branch March 11, 2024 18:49
@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
-->
grusev pushed a commit that referenced this pull request Nov 25, 2024
@muhammadhamzasajjad
#447 Mongo Exceptions Normalization (#1411)
fe805da 
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
-->
grusev pushed a commit that referenced this pull request Nov 25, 2024
@muhammadhamzasajjad
LMDB exception normalization with mock client (#1414)
ab3caf8 
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

@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
2 participants
@muhammadhamzasajjad @poodlewars