Skip to content

Commit

Permalink
Issue #1017 Only warn if the "base" LMDB env is opened twice
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
poodlewars committed Oct 31, 2023
1 parent d633f9b commit c9a0b55
Show file tree
Hide file tree
Showing 3 changed files with 21 additions and 22 deletions.
34 changes: 13 additions & 21 deletions cpp/arcticdb/storage/lmdb/lmdb_storage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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={}",
Expand Down Expand Up @@ -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()));
}
}

Expand Down
2 changes: 1 addition & 1 deletion cpp/arcticdb/storage/lmdb/lmdb_storage.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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<KeySegmentPair>&& kvs, ::lmdb::txn& txn);
Expand Down
7 changes: 7 additions & 0 deletions python/tests/integration/arcticdb/test_lmdb.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"]

0 comments on commit c9a0b55

Please sign in to comment.