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

LMDB exception normalization with mock client #1414

Merged
merged 5 commits into from
Mar 14, 2024

Conversation

muhammadhamzasajjad
Copy link
Collaborator

@muhammadhamzasajjad muhammadhamzasajjad commented Mar 12, 2024

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, 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

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 self-assigned this Mar 12, 2024
@muhammadhamzasajjad muhammadhamzasajjad added the enhancement New feature or request label Mar 12, 2024
@muhammadhamzasajjad muhammadhamzasajjad marked this pull request as ready for review March 12, 2024 17:43
cpp/arcticdb/CMakeLists.txt Outdated Show resolved Hide resolved
cpp/arcticdb/storage/lmdb/lmdb_mock_client.hpp Outdated Show resolved Hide resolved
cpp/arcticdb/storage/lmdb/lmdb_real_client.cpp Outdated Show resolved Hide resolved
cpp/arcticdb/util/error_code.hpp Show resolved Hide resolved
@vasil-pashov
Copy link
Collaborator

LGTM given CI passes.

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.

Please squash commits when you rebase this in.

@muhammadhamzasajjad muhammadhamzasajjad merged commit 882e73e into master Mar 14, 2024
111 checks passed
@muhammadhamzasajjad muhammadhamzasajjad deleted the lmdb_exceptions branch March 14, 2024 12:40
grusev pushed a commit that referenced this pull request Nov 25, 2024
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
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use specific exception type for certain kinds of storage failures
3 participants