diff --git a/cpp/arcticdb/storage/s3/detail-inl.hpp b/cpp/arcticdb/storage/s3/detail-inl.hpp index 5e9128e79f..ceb3fc29cb 100644 --- a/cpp/arcticdb/storage/s3/detail-inl.hpp +++ b/cpp/arcticdb/storage/s3/detail-inl.hpp @@ -168,6 +168,7 @@ namespace s3 { template void do_read_impl(Composite &&ks, const ReadVisitor &visitor, + folly::Function key_decoder, const std::string &root_folder, const std::string &bucket_name, const S3ClientWrapper &s3_client, @@ -179,7 +180,7 @@ namespace s3 { (fg::from(ks.as_range()) | fg::move | fg::groupBy(fmt_db)).foreach( [&s3_client, &bucket_name, &root_folder, b = std::move(bucketizer), &visitor, &keys_not_found, - opts = opts](auto &&group) { + &key_decoder, opts = opts](auto &&group) { for (auto &k: group.values()) { auto key_type_dir = key_type_folder(root_folder, variant_key_type(k)); @@ -189,13 +190,14 @@ namespace s3 { s3_object_name, bucket_name); + auto unencoded_key = key_decoder(std::move(k)); if (get_object_result.is_success()) { ARCTICDB_SUBSAMPLE(S3StorageVisitSegment, 0) - visitor(k, std::move(get_object_result.get_output())); + visitor(unencoded_key, std::move(get_object_result.get_output())); - ARCTICDB_DEBUG(log::storage(), "Read key {}: {}", variant_key_type(k), - variant_key_view(k)); + ARCTICDB_DEBUG(log::storage(), "Read key {}: {}", variant_key_type(unencoded_key), + variant_key_view(unencoded_key)); } else { auto &error = get_object_result.get_error(); raise_if_unexpected_error(error, s3_object_name); @@ -203,11 +205,11 @@ namespace s3 { log::storage().log( opts.dont_warn_about_missing_key ? spdlog::level::debug : spdlog::level::warn, "Failed to find segment for key '{}' {}: {}", - variant_key_view(k), + variant_key_view(unencoded_key), error.GetExceptionName().c_str(), error.GetMessage().c_str()); - keys_not_found.push_back(k); + keys_not_found.push_back(unencoded_key); } } }); diff --git a/cpp/arcticdb/storage/s3/nfs_backed_storage.cpp b/cpp/arcticdb/storage/s3/nfs_backed_storage.cpp index 236c8d9f96..f065b966cd 100644 --- a/cpp/arcticdb/storage/s3/nfs_backed_storage.cpp +++ b/cpp/arcticdb/storage/s3/nfs_backed_storage.cpp @@ -171,15 +171,11 @@ void NfsBackedStorage::do_update(Composite&& kvs, UpdateOpts) { } void NfsBackedStorage::do_read(Composite&& ks, const ReadVisitor& visitor, ReadKeyOpts opts) { - auto func = [visitor] (const VariantKey& k, Segment&& seg) mutable { - visitor(unencode_object_id(k), std::move(seg)); - }; - auto enc = ks.transform([] (auto&& key) { return encode_object_id(key); }); - s3::detail::do_read_impl(std::move(enc), func, root_folder_, bucket_name_, *s3_client_, NfsBucketizer{}, opts); + s3::detail::do_read_impl(std::move(enc), visitor, unencode_object_id, root_folder_, bucket_name_, *s3_client_, NfsBucketizer{}, opts); } void NfsBackedStorage::do_remove(Composite&& ks, RemoveOpts) { diff --git a/cpp/arcticdb/storage/s3/s3_storage.cpp b/cpp/arcticdb/storage/s3/s3_storage.cpp index 5964161e80..9867ce8c79 100644 --- a/cpp/arcticdb/storage/s3/s3_storage.cpp +++ b/cpp/arcticdb/storage/s3/s3_storage.cpp @@ -62,7 +62,7 @@ void S3Storage::do_update(Composite&& kvs, UpdateOpts) { } void S3Storage::do_read(Composite&& ks, const ReadVisitor& visitor, ReadKeyOpts opts) { - detail::do_read_impl(std::move(ks), visitor, root_folder_, bucket_name_, *s3_client_, FlatBucketizer{}, opts); + detail::do_read_impl(std::move(ks), visitor, folly::identity, root_folder_, bucket_name_, *s3_client_, FlatBucketizer{}, opts); } void S3Storage::do_remove(Composite&& ks, RemoveOpts) { diff --git a/cpp/arcticdb/storage/test/test_s3_storage.cpp b/cpp/arcticdb/storage/test/test_s3_storage.cpp index 810243b530..fb7177c33f 100644 --- a/cpp/arcticdb/storage/test/test_s3_storage.cpp +++ b/cpp/arcticdb/storage/test/test_s3_storage.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include @@ -225,7 +226,38 @@ class S3StorageFixture : public testing::Test { S3Storage store; }; -TEST_F(S3StorageFixture, test_key_exists){ +arcticdb::storage::nfs_backed::NfsBackedStorage::Config get_test_nfs_config() { + arcticdb::storage::nfs_backed::NfsBackedStorage::Config cfg; + cfg.set_use_mock_storage_for_testing(true); + return cfg; +} + +class NfsStorageFixture : public testing::Test { +protected: + NfsStorageFixture(): + store(LibraryPath("lib", '.'), OpenMode::DELETE, get_test_nfs_config()) + {} + + arcticdb::storage::nfs_backed::NfsBackedStorage store; +}; + +class S3AndNfsStorageFixture : public testing::TestWithParam { +public: + std::unique_ptr get_storage() { + LibraryPath lp{"lib"}; + if (GetParam() == "nfs") { + return std::make_unique( + lp, OpenMode::DELETE, get_test_nfs_config()); + } else if (GetParam() == "s3") { + return std::make_unique( + lp, OpenMode::DELETE, S3Settings(get_test_s3_config())); + } else { + util::raise_rte("Unexpected fixture type {}", GetParam()); + } + } +}; + +TEST_F(S3StorageFixture, test_key_exists) { write_in_store(store, "symbol"); ASSERT_TRUE(exists_in_store(store, "symbol")); @@ -245,6 +277,23 @@ TEST_F(S3StorageFixture, test_read){ UnexpectedS3ErrorException); } +TEST_P(S3AndNfsStorageFixture, test_read_missing_key_in_exception){ + auto s = get_storage(); + auto& store = *s; + + try { + read_in_store(store, "snap-not-present", KeyType::SNAPSHOT_REF); + FAIL(); + } catch (KeyNotFoundException& e) { + auto keys = e.keys().as_range(); + ASSERT_EQ(keys.size(), 1); + const auto& key = keys.at(0); + ASSERT_EQ(variant_key_id(key), StreamId{"snap-not-present"}); + } +} + +INSTANTIATE_TEST_SUITE_P(S3AndNfs, S3AndNfsStorageFixture, testing::Values("s3", "nfs")); + TEST_F(S3StorageFixture, test_write){ write_in_store(store, "symbol"); ASSERT_THROW( diff --git a/cpp/arcticdb/version/snapshot.cpp b/cpp/arcticdb/version/snapshot.cpp index 6afdaf4afd..50359d0f03 100644 --- a/cpp/arcticdb/version/snapshot.cpp +++ b/cpp/arcticdb/version/snapshot.cpp @@ -107,7 +107,9 @@ void iterate_snapshots(const std::shared_ptr& store, folly::Function