Warning for LMDB when library reopened #1000
Conversation
poodlewars
changed the title
poodlewars
force-pushed
the
lmdb-warning-reopen
branch
2 times, most recently
from
a723a8d
to
d54aec8
Compare
An alternative approach for this PR was to use psutil to check which files are held open by the process. However psutil is actually not used for anything important, so I chose instead to remove this Python dependency and make ArcticDB installation easier for users as a result.
This is unsafe as it leaves the LMDB library temporarily opened twice. There's a similar problem for RocksDB too.
Tests and compilation fixes
poodlewars
force-pushed
the
lmdb-warning-reopen
branch
from
d54aec8
to
12778c3
Compare
poodlewars
changed the title
…es are actually usable.
| @@ -285,8 +285,8 @@ def create_library(self, name: str, library_options: Optional[LibraryOptions] = | |||
| if library_options is None: | |||
| library_options = LibraryOptions() | |||
|
|
|||
| library = self._library_adapter.create_library(name, library_options) | |||
| self._library_manager.write_library_config(library._lib_cfg, name, self._library_adapter.get_masking_override()) | |||
| cfg = self._library_adapter.get_library_config(name, library_options) | |||
There was a problem hiding this comment.
Just noting that this will fix the PR for the RocksDB Python API #991
| ac = Arctic(f"lmdb://{tmpdir}") | ||
|
|
||
| del ac | ||
| # should not warn |
There was a problem hiding this comment.
Checking the warning is actually produced is carried out manually then?
There was a problem hiding this comment.
Yeah, checking the logging was super difficult and didn't seem worth it
| def create_store_from_config( | ||
| cls, cfg, env, lib_name, open_mode=OpenMode.DELETE, encoding_version=EncodingVersion.V1 | ||
| @staticmethod | ||
| def create_library_config( |
There was a problem hiding this comment.
This makes arctic.py much clearer now. There was no need to create a temporary Library just to get its config and delete it.
| LmdbStorage::LmdbStorage(LmdbStorage&& other) noexcept | ||
| : Storage(std::move(static_cast<Storage&>(other))), | ||
| write_mutex_(std::move(other.write_mutex_)), | ||
| env_(std::move(other.env_)), | ||
| dbi_by_key_type_(std::move(other.dbi_by_key_type_)), | ||
| lib_dir_(std::move(other.lib_dir_)) { | ||
| other.lib_dir_ = ""; | ||
| } |
There was a problem hiding this comment.
Just noting that when Storages are made no move or copy in #991 I'll remove this move constructor. Is that going to be fine? It might mean the check of !lib_dir_.empty() in the destructor can go also, because lib_dir should always be passed something in the constructor.
There was a problem hiding this comment.
Proof of the pudding will be in the eating, but yeah that sounds good.
Reference Issues/PRs
#889 #520 #981
Also see PR #990 for which this is a followup.
What does this implement or fix?
It's only safe to open an LMDB database once from a given process. LMDB document this. It's because they rely on
flocklocking on the files, which is re-entrant within a process. This warns the user if they have multiple live Arctic instances over the same LMDB database.We also:
psutilPython dep (which I considered using as an alternative solution here)_arctic_cfgspecial string down in the Native extensionArcticAPI where it needlessly created an extraNativeVersionStorewhen creating a library. The new warning flagged this up.