447 Consistently use `KeyNotFoundException`, `DuplicateKeyException` and `PermissionException` in `Storage`s #584

qc00 · 2023-07-12T13:06:53Z

Error handling behaviour comparison

Storage	Function	Throws	For situation	Key in error msg	Changed in this PR
LMDB	Common	lmdb::error	transaction/low-level failure	None	No
	write	DuplicateKeyException	overwriting AtomKey	One	No
	write	std::runtime_error	other write failures	One	No
	update	KeyNotFoundException	upsert=false on existing key	All	Added user-friendly msg (Instead of "Composite: ")
	read	KeyNotFoundException	key not found	All	No
	key_exists	-	-	-	No
	remove	KeyNotFoundException	keys already deleted	All	No
	iterate	(None)	-	-	No
memory	write	util::check	overwriting AtomKey	One	DuplicateKeyException
	update	util::check_rte	upsert=false on existing key	One	KeyNotFoundException
	read	KeyNotFoundException		One	No
	key_exists	-	-	-	No
	remove	remove	keys already deleted	One	KeyNotFoundException
	iterate	-	-	-	No
Mongo	Common	Mongo may throw exceptions other than those below			No
	Common	util::check / std::runtime_error	Mongo "write concern" timed out	One	Retryable E_MONGO_BULK_OP_NO_REPLY
	write update	mongocxx::operation_exception	Any other errors	One	E_MONGO_OP_FAILED with the server message
	update	util::check	The update count does not match the input	One	KeyNotFoundException with user-friendly message
	read	KeyNotFoundException	key not found	One	No
		KeyNotFoundException	Any other error from Mongo	One	E_MONGO_OP_FAILED with the server message
		KeyNotFoundException	Exceptions from our code/assertions	-	Propagates the original exception
	remove	mongocxx::operation_exception	Any error	One	KeyNotFoundException, All. Server messages are logged.
	key_exists	(Only logging)	Any error other than key not found	-	E_MONGO_OP_FAILED
	iterate	mongocxx::operation_exception	Any error from Mongo	None	No
S3	write update	util::raise_rte	Any request failure (But no handling for upsert=false in S3)	One	handle_common_errors
	read / exists	KeyNotFoundException / Debug log and return false	key not found	All	No
			NO_SUCH_BUCKET	All	E_UNEXPECTED_S3_ERROR, One
			INVALID_ACCESS_KEY_ID / Permission issue	All	E_PERMISSION, One
		UnexpectedS3ErrorException	SIGNATURE_DOES_NOT_MATCH	One	E_PERMISSION
	remove	KeyNotFoundException	Bucket exists but not key	All	No
	remove	(None)	All other errors (no bucket, creds, network, etc.)	-	Also KeyNotFoundException
	iterate	(Only warning)	Any error	-	Same as read

Related improvements

provide meaningful messages for mongo exceptions
Systematic tests for storage exceptions
Lift out storage fixtures with new abilities to simulate errors:
- shutting down the fixture early
- multiple users
- changing bucket permissions
Expanding library_tool to expose all Storage methods
Fix minor bug in Segment c'tor

+ warn if AWS auth is being used and machine identity is disabled + Refactor S3 fixtures so various errors can be simulated * Removed pytest-server-fixtures dependency which is a bit old and we only use function

Also provide meaningful messages for mongo exceptions

poodlewars

This is great! ⭐ ⭐ ⭐ ⭐ ⭐

Would be good to add tests verifying that the table in your PR description is correct.

poodlewars · 2023-08-02T14:19:00Z

cpp/arcticdb/python/python_module.cpp

@@ -215,7 +215,8 @@ void register_error_code_ecosystem(py::module& m, py::exception<arcticdb::Arctic

 py::register_exception<SchemaException>(m, "SchemaException", compat_exception.ptr());
 py::register_exception<NormalizationException>(m, "NormalizationException", compat_exception.ptr());
- py::register_exception<StorageException>(m, "StorageException", compat_exception.ptr());
+ auto& storage_exception = py::register_exception<StorageException>(m, "StorageException", compat_exception.ptr());
+ py::register_exception<StorageRetryableException>(m, "StorageRetryableException", storage_exception.ptr());


Any docs update needed? eg

ArcticDB/docs/mkdocs/docs/error_messages.md

Line 151 in 3330bf3

## Exception Hierarchy

poodlewars · 2023-08-02T14:24:22Z

cpp/arcticdb/storage/mongo/mongo_client.cpp

+ result.has_value(), "Mongo did not acknowledge write for key {}", kv.key_view());
+ if (!upsert && result->modified_count() == 0) {
+ throw KeyNotFoundException(std::move(kv.variant_key()),
+ "update_segment called with upsert=false on non-exist key: {}");


Suggested change

"update_segment called with upsert=false on non-exist key: {}");

"update_segment called with upsert=false on non-existent key: {}");

poodlewars · 2023-08-02T14:25:20Z

cpp/arcticdb/util/error_code.hpp

@@ -62,6 +64,7 @@ inline std::unordered_map<ErrorCategory, const char*> get_error_category_names()
 ERROR_CODE(2003, E_INCOMPATIBLE_INDEX) \
 ERROR_CODE(2004, E_WRONG_SHAPE) \
 ERROR_CODE(3000, E_NO_SUCH_VERSION) \
+ ERROR_CODE(3001, E_SYMBOL_NOT_FOUND) \


docs for these, error_messages.md

cpp/arcticdb/toolbox/library_tool.hpp

poodlewars · 2023-08-02T14:27:49Z

cpp/arcticdb/toolbox/library_tool.cpp

-void LibraryTool::write(VariantKey key, Segment segment) {
- storage::KeySegmentPair kv{std::move(key), std::move(segment)};
- lib_->write(Composite<storage::KeySegmentPair>{std::move(kv)});
+void LibraryTool::write_batch(std::vector<VariantKey> keys, std::vector<Segment> segments) {


(minor) Might be friendlier to assert keys and segments are the same length

poodlewars · 2023-08-02T15:11:27Z

cpp/arcticdb/storage/s3/s3_storage-inl.hpp

+ return; // Caller need to handle these
+ }
+
+#define S3_ERROR_FMT " error while {} {}: S3Error#{} {}: {}", \


No real need for this to be a macro is there? Just a normal function would be fine?

poodlewars · 2023-08-02T15:20:38Z

cpp/arcticdb/storage/s3/s3_storage-inl.hpp

-
-inline bool is_expected_error_type(Aws::S3::S3Errors err) {
- return err == Aws::S3::S3Errors::NO_SUCH_KEY
- || err == Aws::S3::S3Errors::NO_SUCH_BUCKET


We've lost the special handling for NO_SUCH_BUCKET in this PR, I assume because you check it's accessible when the Arctic instance is created?

What if the bucket is deleted after the Arctic instance is created?

poodlewars · 2023-08-02T15:36:14Z

cpp/arcticdb/storage/s3/s3_storage-inl.hpp

-
- }
- else {
- const auto& error = list_objects_outcome.GetError();


I spent a long time thinking about this one. Looks like we are similar to the old implementation in that we stop the paging after the first error. Difference is that now sometimes we throw. Are callers of this method able to cope with iterate type throwing?

poodlewars · 2023-08-02T15:40:27Z

cpp/arcticdb/storage/s3/s3_storage.cpp

+ if (!outcome.IsSuccess()) {
+ auto& error = outcome.GetError();
+
+#define BUCKET_LOG(level, msg, ...) log::storage().level(msg "\nHTTP Status: {}. Server response: {}", \


No macro please

poodlewars · 2023-08-02T15:43:52Z

cpp/arcticdb/storage/s3/s3_storage.cpp

+ auto wait = std::chrono::milliseconds(ConfigsMap::instance()->get_int("S3Storage.CheckBucketMaxWait", 1000));
+ if (future.wait_for(wait) == std::future_status::ready) {
+ auto outcome = future.get();
+ if (!outcome.IsSuccess()) {


if (outcome.IsSuccess()) { return true; } // log some stuff return false;

is clearer IMO. There's only one way we can return true here and the code could make that more obvious.

vasil-pashov · 2023-08-03T15:33:01Z

cpp/arcticdb/storage/lmdb/lmdb_storage-inl.hpp

@@ -50,6 +50,7 @@ inline void LmdbStorage::do_write_internal(Composite<KeySegmentPair>&& kvs, ::lm
 ARCTICDB_SUBSAMPLE(LmdbPut, 0)
 int res = ::mdb_put(txn.handle(), dbi.handle(), &mdb_key, &mdb_val, MDB_RESERVE | overwrite_flag);
 if (res == MDB_KEYEXIST) {
+ // Since LMDB is in-memory, we can efficiently detect: (see its doc for details)


I can quite follow this comment. What can we efficiently detect?

vasil-pashov · 2023-08-03T15:35:50Z

cpp/arcticdb/storage/lmdb/lmdb_storage-inl.hpp

@@ -85,15 +86,16 @@ inline void LmdbStorage::do_update(Composite<KeySegmentPair>&& kvs, UpdateOpts o
 if(!failed_deletes.empty()) {
 ARCTICDB_SUBSAMPLE(LmdbStorageCommit, 0)
 txn.commit();
- throw KeyNotFoundException(Composite<VariantKey>(std::move(failed_deletes)));
+ throw KeyNotFoundException(Composite<VariantKey>(std::move(failed_deletes)),
+ "do_update called with upsert=false on non-exist key(s): {}");


Suggested change

"do_update called with upsert=false on non-exist key(s): {}");

"do_update called with upsert=false on non-existent key(s): {}");

vasil-pashov · 2023-08-03T15:53:10Z

cpp/arcticdb/storage/memory/memory_storage-inl.hpp

- util::check_rte(opts.upsert_ || it != key_vec.end(), "update called with upsert=false but key does not exist");
+ if (!opts.upsert_ && it == key_vec.end()) {
+ throw KeyNotFoundException(std::move(kv.variant_key()),
+ "update called with upsert=false but key does not exist: {}");


The error message is different than the error message for LMDB. I think we should unify them.

(Minor) I wouldn't move kv.variant_key() but copy it. We're already doing the slow thing throwing, so I don't see any benefit of moving a string. If we don't move the debugging will be slightly better as the key will exist in the stack trace.

vasil-pashov · 2023-08-03T16:00:04Z

cpp/arcticdb/storage/mongo/mongo_client.cpp

+ * https://github.com/mongodb/mongo-cxx-driver/blob/master/src/mongocxx/exception/error_code.hpp for constants
+ */
+
+/** Mongo often fail to set the what() string, so has to manually format the exception */


Suggested change

/** Mongo often fail to set the what() string, so has to manually format the exception */

/** Mongo often fails to set the what() string, so has to manually format the exception */

vasil-pashov · 2023-08-03T16:03:48Z

cpp/arcticdb/storage/mongo/mongo_client.cpp

+
+/** Mongo often fail to set the what() string, so has to manually format the exception */
+[[noreturn]] void translate_operation_exception(const mongocxx::operation_exception& e) {
+ std::string json;


(Minor) Isn't the naming slightly misleading? As far as I understand bsoncxx::to_json(e.raw_server_error()->view()); would return a stringified JSON, but fmt::format("(Failed to format server response either: {})", e.what()); "No response from server"; are not JSONs.

vasil-pashov · 2023-08-03T16:06:01Z

cpp/arcticdb/storage/mongo/mongo_client.cpp

- util::check(bool(result), "Mongo error while putting key {}", kv.key_view());
+ try {
+ auto result = bulk_write.execute();
+ // We don't use the "mongoc_write_concern_set_w" that allows nullopt to be returned, but check it anyway:


Is there a reason to check it? Maybe a comment on why we check it anyway.

vasil-pashov · 2023-08-03T16:13:15Z

cpp/arcticdb/storage/mongo/mongo_client.cpp

+ if(StorageFailureSimulator::instance()->configured())
+ StorageFailureSimulator::instance()->go(FailureType::READ);


(Personal preference) I believe that wrapping if/else/for/while bodies in {} even for one liners is more readable.

vasil-pashov · 2023-08-03T16:20:12Z

cpp/arcticdb/storage/mongo/mongo_client.cpp

 auto size = doc["total_size"].get_int64().value;
- entity::VariantKey stored_key{ detail::variant_key_from_document(doc, key) };
+ entity::VariantKey stored_key{detail::variant_key_from_document(doc, key)};
 util::check(stored_key == key, "Key mismatch: {} != {}");


This is a general question not related to this PR in particular. Do we want to leak E_ASSERTION_FAILURE exceptions? AFAIK we use this in places where people would use cassert's function assert so do we want people to be able to see this in "raw" form and why don't we use casserts in general?

vasil-pashov · 2023-08-03T16:23:43Z

cpp/arcticdb/storage/mongo/mongo_client.cpp

- catch(const std::exception& ex) {
- log::storage().info("Segment read error: {}", ex.what());
- throw storage::KeyNotFoundException{Composite<VariantKey>{VariantKey{key}}};
+ } catch(const mongocxx::operation_exception& e) {


We're not catching std::exception anymore. What was the thing that used to throw anything inheriting from std::exception?

vasil-pashov · 2023-08-03T16:25:15Z

cpp/arcticdb/storage/mongo/mongo_client.cpp

- } else {
- result = collection.delete_one(document{} << "key" << fmt::format("{}", key) << "stream_id" <<
- fmt::format("{}", variant_key_id(key)) << finalize);
+ auto filter = document{} << "key" << fmt::format("{}", key) << "stream_id" << fmt::format("{}", variant_key_id(key))


(Minor) I think one large fmt::format (if possible) would be easier to read

vasil-pashov · 2023-08-04T07:26:01Z

cpp/arcticdb/storage/s3/s3_storage.cpp

+ } else {
+ log::storage().info("Unable to determine if the bucket is accessible. Request timed out.");
+ }
+ return false;


What does false stand for? It's no quite obvious to me from the name of the function?

vasil-pashov · 2023-08-04T07:39:12Z

cpp/arcticdb/toolbox/library_tool.cpp

+void LibraryTool::write_batch(std::vector<VariantKey> keys, std::vector<Segment> segments) {
+ Composite<storage::KeySegmentPair> composite;
+ for (size_t i = 0; i < keys.size(); i++) {
+ composite.push_back({std::move(keys[i]), std::move(segments.at(i))});


Is this performance critical? Calling at is slightly slower due to bounds checks. Do we want it to throw? Who's supposed to handle it in case of out_of_bounds exception?

vasil-pashov · 2023-08-04T07:45:14Z

python/arcticdb/util/test.py

@@ -195,6 +197,14 @@ def configure_test_logger(level="INFO"):
 configure(get_test_logger_config(level=level, outputs=outputs), force=True)


+def gracefully_terminate_process(p):
+ p.terminate()
+ if sys.platform != "win32":


What about windows?
Is sigkill considered as graceful termination seems rather violent to me?

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.

Implementation of memory storage specific exceptions normalization. This PR is a refactor of #584.

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>

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.

This performs normalization of RocksDB storage exceptions as per #584.

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>

qc00 and others added 4 commits July 4, 2023 16:15

#473 Warn if bucket cannot be accessed

e9badb0

+ warn if AWS auth is being used and machine identity is disabled + Refactor S3 fixtures so various errors can be simulated * Removed pytest-server-fixtures dependency which is a bit old and we only use function

#447 Consistently use KeyNotFoundException and DuplicateKeyException

4707a30

Also provide meaningful messages for mongo exceptions

#447 Use PermissionException in more places

7444857

wip

81934e9

poodlewars requested review from vasil-pashov and poodlewars August 1, 2023 16:58

poodlewars reviewed Aug 2, 2023

View reviewed changes

vasil-pashov reviewed Aug 3, 2023

View reviewed changes

vasil-pashov reviewed Aug 4, 2023

View reviewed changes

phoebusm mentioned this pull request Nov 16, 2023

Add preliminary change for slowdown error test #1064

Merged

5 tasks

poodlewars added the enhancement New feature or request label Dec 29, 2023

joe-iddon mentioned this pull request Jan 11, 2024

Add frequently used items at the top level of arcticdb #1219

Merged

muhammadhamzasajjad mentioned this pull request Jan 31, 2024

#447 LMDB Exceptions Normalization #1285

Merged

This was referenced Feb 2, 2024

#447 Memory Storage Exception Normalization #1297

Merged

#447 S3 Storage Exceptions Handling #1304

Merged

muhammadhamzasajjad added a commit that referenced this pull request Feb 8, 2024

#447 Memory Storage Exception Normalization (#1297)

da288af

Implementation of memory storage specific exceptions normalization. This PR is a refactor of #584.

muhammadhamzasajjad mentioned this pull request Feb 19, 2024

#447 Azure Storage Exceptions Normalization #1344

Merged

5 tasks

muhammadhamzasajjad mentioned this pull request Feb 23, 2024

#447 Exception normalization for RocksDB #1360

Merged

5 tasks

muhammadhamzasajjad added a commit that referenced this pull request Feb 28, 2024

#447 Exception normalization for RocksDB (#1360)

8db4515

This performs normalization of RocksDB storage exceptions as per #584.

muhammadhamzasajjad mentioned this pull request Mar 11, 2024

#447 Mongo Exceptions Normalization #1411

Merged

5 tasks

poodlewars closed this Aug 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

447 Consistently use `KeyNotFoundException`, `DuplicateKeyException` and `PermissionException` in `Storage`s #584

447 Consistently use `KeyNotFoundException`, `DuplicateKeyException` and `PermissionException` in `Storage`s #584

qc00 commented Jul 12, 2023 •

edited

Loading

poodlewars left a comment

poodlewars Aug 2, 2023

poodlewars Aug 2, 2023

poodlewars Aug 2, 2023

poodlewars Aug 2, 2023

poodlewars Aug 2, 2023

vasil-pashov Aug 4, 2023

poodlewars Aug 2, 2023

poodlewars Aug 2, 2023

poodlewars Aug 2, 2023

poodlewars Aug 2, 2023

vasil-pashov Aug 3, 2023

vasil-pashov Aug 3, 2023

vasil-pashov Aug 3, 2023

vasil-pashov Aug 3, 2023

vasil-pashov Aug 3, 2023

vasil-pashov Aug 3, 2023

vasil-pashov Aug 3, 2023

vasil-pashov Aug 3, 2023

vasil-pashov Aug 3, 2023

vasil-pashov Aug 3, 2023

vasil-pashov Aug 4, 2023

vasil-pashov Aug 4, 2023

vasil-pashov Aug 4, 2023

	"update_segment called with upsert=false on non-exist key: {}");
	"update_segment called with upsert=false on non-existent key: {}");

	"do_update called with upsert=false on non-exist key(s): {}");
	"do_update called with upsert=false on non-existent key(s): {}");

	/** Mongo often fail to set the what() string, so has to manually format the exception */
	/** Mongo often fails to set the what() string, so has to manually format the exception */

		if(StorageFailureSimulator::instance()->configured())
		StorageFailureSimulator::instance()->go(FailureType::READ);

447 Consistently use KeyNotFoundException, DuplicateKeyException and PermissionException in Storages #584

447 Consistently use KeyNotFoundException, DuplicateKeyException and PermissionException in Storages #584

Conversation

qc00 commented Jul 12, 2023 • edited Loading

Error handling behaviour comparison

Related improvements

poodlewars left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

447 Consistently use `KeyNotFoundException`, `DuplicateKeyException` and `PermissionException` in `Storage`s #584

447 Consistently use `KeyNotFoundException`, `DuplicateKeyException` and `PermissionException` in `Storage`s #584

qc00 commented Jul 12, 2023 •

edited

Loading