From 6f741d202df6e4cf6c05dfb51fdab98fcdf820e0 Mon Sep 17 00:00:00 2001 From: Muhammad Hamza Sajjad <45758804+muhammadhamzasajjad@users.noreply.github.com> Date: Fri, 2 Feb 2024 14:35:31 +0000 Subject: [PATCH] #447 LMDB Exceptions Normalization (#1285) 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. --- cpp/arcticdb/CMakeLists.txt | 1 + cpp/arcticdb/storage/lmdb/lmdb_storage.cpp | 3 +- cpp/arcticdb/storage/storage.hpp | 9 +- .../storage/test/test_storage_exceptions.cpp | 171 ++++++++++++++++++ 4 files changed, 181 insertions(+), 3 deletions(-) create mode 100644 cpp/arcticdb/storage/test/test_storage_exceptions.cpp diff --git a/cpp/arcticdb/CMakeLists.txt b/cpp/arcticdb/CMakeLists.txt index 5f22fbb155..f03c8b563e 100644 --- a/cpp/arcticdb/CMakeLists.txt +++ b/cpp/arcticdb/CMakeLists.txt @@ -792,6 +792,7 @@ if(${TEST}) version/test/test_version_store.cpp version/test/test_sorting_info_state_machine.cpp version/test/version_map_model.hpp + storage/test/test_storage_exceptions.cpp ) set(EXECUTABLE_PERMS OWNER_WRITE OWNER_READ OWNER_EXECUTE GROUP_READ GROUP_EXECUTE WORLD_READ WORLD_EXECUTE) # 755 diff --git a/cpp/arcticdb/storage/lmdb/lmdb_storage.cpp b/cpp/arcticdb/storage/lmdb/lmdb_storage.cpp index 30c86ee873..015726a605 100644 --- a/cpp/arcticdb/storage/lmdb/lmdb_storage.cpp +++ b/cpp/arcticdb/storage/lmdb/lmdb_storage.cpp @@ -92,7 +92,8 @@ void LmdbStorage::do_update(Composite&& kvs, UpdateOpts opts) { if(!failed_deletes.empty()) { ARCTICDB_SUBSAMPLE(LmdbStorageCommit, 0) txn.commit(); - throw KeyNotFoundException(Composite(std::move(failed_deletes))); + std::string err_message = fmt::format("do_update called with upsert=false on non-existent key(s): {}", failed_deletes); + throw KeyNotFoundException(Composite(std::move(failed_deletes)), err_message); } do_write_internal(std::move(kvs), txn); ARCTICDB_SUBSAMPLE(LmdbStorageCommit, 0) diff --git a/cpp/arcticdb/storage/storage.hpp b/cpp/arcticdb/storage/storage.hpp index df8484e2f5..d51d723944 100644 --- a/cpp/arcticdb/storage/storage.hpp +++ b/cpp/arcticdb/storage/storage.hpp @@ -61,13 +61,18 @@ class NoDataFoundException : public ArcticCategorizedException key_; }; -class KeyNotFoundException : public ArcticSpecificException { +class KeyNotFoundException : public ArcticSpecificException { public: explicit KeyNotFoundException(Composite&& keys) : - ArcticSpecificException(fmt::format("{}", keys)), + ArcticSpecificException(fmt::format("Not found: {}", keys)), keys_(std::make_shared>(std::move(keys))) { } + explicit KeyNotFoundException(Composite&& keys, std::string err_output) : + ArcticSpecificException(err_output), + keys_(std::make_shared>(std::move(keys))) { + } + Composite& keys() { return *keys_; } diff --git a/cpp/arcticdb/storage/test/test_storage_exceptions.cpp b/cpp/arcticdb/storage/test/test_storage_exceptions.cpp new file mode 100644 index 0000000000..9211e4ade4 --- /dev/null +++ b/cpp/arcticdb/storage/test/test_storage_exceptions.cpp @@ -0,0 +1,171 @@ +/* Copyright 2023 Man Group Operations Limited + * + * Use of this software is governed by the Business Source License 1.1 included in the file licenses/BSL.txt. + * + * As of the Change Date specified in that file, in accordance with the Business Source License, use of this software will be governed by the Apache License, version 2.0. + */ + +#include + +#include +#include +#include +#include + +#include +#include + +inline const fs::path TEST_DATABASES_PATH = "./test_databases"; + +class StorageFactory { +public: + virtual ~StorageFactory() = default; + virtual std::unique_ptr create() = 0; + + virtual void setup() { } + virtual void clear_setup() { } +}; + +class LMDBStorageFactory : public StorageFactory { +private: + uint64_t map_size; + +public: + LMDBStorageFactory() : map_size(128ULL * (1ULL << 20) /* 128MB */) { } + + explicit LMDBStorageFactory(uint64_t map_size) : map_size(map_size) { } + + std::unique_ptr create() override { + arcticdb::proto::lmdb_storage::Config cfg; + + fs::path db_name = "test_lmdb"; + cfg.set_path((TEST_DATABASES_PATH / db_name).generic_string()); + cfg.set_map_size(map_size); + cfg.set_recreate_if_exists(true); + + arcticdb::storage::LibraryPath library_path{"a", "b"}; + + return std::make_unique(library_path, arcticdb::storage::OpenMode::WRITE, cfg); + } + + void setup() override { + if (!fs::exists(TEST_DATABASES_PATH)) { + fs::create_directories(TEST_DATABASES_PATH); + } + } + + void clear_setup() override { + if (fs::exists(TEST_DATABASES_PATH)) { + fs::remove_all(TEST_DATABASES_PATH); + } + } +}; + +// Generic tests that run with all types of storages + +class GenericStorageTest : public ::testing::TestWithParam> { +protected: + std::unique_ptr storage; + + void SetUp() override { + GetParam()->setup(); + storage = GetParam()->create(); + } + + void TearDown() override { + storage.reset(); + GetParam()->clear_setup(); + } +}; + +TEST_P(GenericStorageTest, WriteDuplicateKeyException) { + arcticdb::entity::AtomKey k = arcticdb::entity::atom_key_builder().gen_id(0).build("sym"); + + arcticdb::storage::KeySegmentPair kv(k); + kv.segment().set_buffer(std::make_shared()); + + storage->write(std::move(kv)); + + ASSERT_TRUE(storage->key_exists(k)); + + arcticdb::storage::KeySegmentPair kv1(k); + kv1.segment().set_buffer(std::make_shared()); + + ASSERT_THROW({ + storage->write(std::move(kv1)); + }, arcticdb::storage::DuplicateKeyException); + +} + +TEST_P(GenericStorageTest, ReadKeyNotFoundException) { + arcticdb::entity::AtomKey k = arcticdb::entity::atom_key_builder().gen_id(0).build("sym"); + + ASSERT_TRUE(!storage->key_exists(k)); + ASSERT_THROW({ + storage->read(k, arcticdb::storage::ReadKeyOpts{}); + }, arcticdb::storage::KeyNotFoundException); + +} + +TEST_P(GenericStorageTest, UpdateKeyNotFoundException) { + arcticdb::entity::AtomKey k = arcticdb::entity::atom_key_builder().gen_id(0).build("sym"); + + arcticdb::storage::KeySegmentPair kv(k); + kv.segment().header().set_start_ts(1234); + kv.segment().set_buffer(std::make_shared()); + + ASSERT_TRUE(!storage->key_exists(k)); + ASSERT_THROW({ + storage->update(std::move(kv), arcticdb::storage::UpdateOpts{}); + }, arcticdb::storage::KeyNotFoundException); + +} + +TEST_P(GenericStorageTest, RemoveKeyNotFoundException) { + arcticdb::entity::AtomKey k = arcticdb::entity::atom_key_builder().gen_id(0).build("sym"); + + ASSERT_TRUE(!storage->key_exists(k)); + ASSERT_THROW({ + storage->remove(k, arcticdb::storage::RemoveOpts{}); + }, arcticdb::storage::KeyNotFoundException); + +} + +INSTANTIATE_TEST_SUITE_P( + AllStoragesCommonTests, + GenericStorageTest, + ::testing::Values(std::make_shared()) +); + +// LMDB Storage specific tests + +class LMDBStorageTestBase : public ::testing::Test { +protected: + void SetUp() override { + if (!fs::exists(TEST_DATABASES_PATH)) { + fs::create_directories(TEST_DATABASES_PATH); + } + } + + void TearDown() override { + if (fs::exists(TEST_DATABASES_PATH)) { + fs::remove_all(TEST_DATABASES_PATH); + } + } +}; + +TEST_F(LMDBStorageTestBase, WriteMapFullError) { + // Create a Storage with 32KB map size + LMDBStorageFactory factory(32ULL * (1ULL << 10)); + auto storage = factory.create(); + + arcticdb::entity::AtomKey k = arcticdb::entity::atom_key_builder().gen_id(0).build("sym"); + arcticdb::storage::KeySegmentPair kv(k); + kv.segment().header().set_start_ts(1234); + kv.segment().set_buffer(std::make_shared(40000)); + + ASSERT_THROW({ + storage->write(std::move(kv)); + }, ::lmdb::map_full_error); + +}