From c9a0b55d4dbed10135798fb20537924a2cfffa76 Mon Sep 17 00:00:00 2001 From: Alex Seaton Date: Tue, 31 Oct 2023 15:57:51 +0000 Subject: [PATCH] Issue #1017 Only warn if the "base" LMDB env is opened twice Otherwise, if a user ignores the first error, they are then spammed with misleading errors on their subsequent interactions. See the Github issue for a couple of examples that this change seeks to avoid. --- cpp/arcticdb/storage/lmdb/lmdb_storage.cpp | 34 +++++++------------ cpp/arcticdb/storage/lmdb/lmdb_storage.hpp | 2 +- .../tests/integration/arcticdb/test_lmdb.py | 7 ++++ 3 files changed, 21 insertions(+), 22 deletions(-) diff --git a/cpp/arcticdb/storage/lmdb/lmdb_storage.cpp b/cpp/arcticdb/storage/lmdb/lmdb_storage.cpp index db179edd28..62d88beb65 100644 --- a/cpp/arcticdb/storage/lmdb/lmdb_storage.cpp +++ b/cpp/arcticdb/storage/lmdb/lmdb_storage.cpp @@ -278,7 +278,7 @@ LmdbStorage::LmdbStorage(const LibraryPath &library_path, OpenMode mode, const C lib_dir_ = root_path / lib_path_str; - warn_if_lmdb_already_open(root_path, lib_path_str); + warn_if_lmdb_already_open(); if (!fs::exists(lib_dir_)) { util::check_arg(mode > OpenMode::READ, "Missing dir {} for lib={}. mode={}", @@ -326,27 +326,19 @@ LmdbStorage::LmdbStorage(const LibraryPath &library_path, OpenMode mode, const C ARCTICDB_DEBUG(log::storage(), "Opened lmdb storage at {} with map size {}", lib_dir_.string(), format_bytes(mapsize)); } -void LmdbStorage::warn_if_lmdb_already_open(const fs::path &root_path, const std::string &lib_path_str) { - uint64_t& count_for_pid = ++times_path_opened[(root_path / lib_path_str).string()]; - if (count_for_pid != 1) { +void LmdbStorage::warn_if_lmdb_already_open() { + uint64_t& count_for_pid = ++times_path_opened[lib_dir_.string()]; + // Only warn for the "base" config library to avoid spamming users with more warnings if they decide to ignore it and continue + if (count_for_pid != 1 && lib_dir_.string().find(CONFIG_LIBRARY_NAME) != std::string::npos) { + std::filesystem::path user_facing_path = lib_dir_; // Strip magic name from warning as it will confuse users - if (lib_dir_.string().find(CONFIG_LIBRARY_NAME) == std::string::npos) { - log::storage().warn(fmt::format( - "LMDB path at {} has already been opened in this process which is not supported by LMDB. " - "To continue safely, you should delete all Library objects over the LMDB path in this " - "process and then try again. This indicates a bug in ArcticDB. Please report at " - "https://github.com/man-group/ArcticDB. Current process ID=[{}]", - lib_dir_.string(), getpid())); - } else { - std::filesystem::path user_facing_path = lib_dir_; - user_facing_path.remove_filename(); - log::storage().warn(fmt::format( - "LMDB path at {} has already been opened in this process which is not supported by LMDB. " - "You should only open a single Arctic instance over a given LMDB path. " - "To continue safely, you should delete this Arctic instance and any others over the LMDB path in this " - "process and then try again. Current process ID=[{}]", - user_facing_path.string(), getpid())); - } + user_facing_path.remove_filename(); + log::storage().warn(fmt::format( + "LMDB path at {} has already been opened in this process which is not supported by LMDB. " + "You should only open a single Arctic instance over a given LMDB path. " + "To continue safely, you should delete this Arctic instance and any others over the LMDB path in this " + "process and then try again. Current process ID=[{}]", + user_facing_path.string(), getpid())); } } diff --git a/cpp/arcticdb/storage/lmdb/lmdb_storage.hpp b/cpp/arcticdb/storage/lmdb/lmdb_storage.hpp index 568db6b6f7..ef2714c764 100644 --- a/cpp/arcticdb/storage/lmdb/lmdb_storage.hpp +++ b/cpp/arcticdb/storage/lmdb/lmdb_storage.hpp @@ -60,7 +60,7 @@ class LmdbStorage final : public Storage { std::string do_key_path(const VariantKey&) const final { return {}; }; - void warn_if_lmdb_already_open(const fs::path &root_path, const std::string &lib_path_str); + void warn_if_lmdb_already_open(); // _internal methods assume the write mutex is already held void do_write_internal(Composite&& kvs, ::lmdb::txn& txn); diff --git a/python/tests/integration/arcticdb/test_lmdb.py b/python/tests/integration/arcticdb/test_lmdb.py index 856dfa4336..92ded00f3f 100644 --- a/python/tests/integration/arcticdb/test_lmdb.py +++ b/python/tests/integration/arcticdb/test_lmdb.py @@ -208,3 +208,10 @@ def test_arctic_instances_across_same_lmdb_multiprocessing(tmpdir): ac["test"].write("a", pd.DataFrame()) with mp.Pool(5) as p: p.starmap(create_arctic_instance, [(tmpdir, i) for i in range(20)]) + + +def test_lmdb_warnings_when_reopened(tmpdir): + ac = Arctic(f"lmdb://{tmpdir}") + lib = ac.create_library("test") + ac = Arctic(f"lmdb://{tmpdir}") # expect to warn + lib = ac["test"]