Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use consistent terminology in Manager implementation and comments #28

Merged
merged 1 commit into from
Jun 5, 2018

Conversation

mykmelez
Copy link
Contributor

@mykmelez mykmelez commented Jun 5, 2018

@ncalexan There's some inconsistency in the terminology used by manager.rs to describe the things it manages.

The Rkv struct wraps an LMDB "environment," while the Store struct wraps an LMDB "database," and manager.rs manages the former, but it sometimes describes them as stores rather than environments. This change ensures we use the terms "environment" and "store" consistently in that module.

Note that I asked @rnewman a related question last month:

Something that has been puzzling me about LMDB recently is that http://www.lmdb.tech/doc/starting.html says, “Because of this, do not mdb_env_open() a file multiple times from a single process. Instead, share the LMDB environment that has opened the file across all threads.” And rkv::Manager enforces this constraint…

But other references, like http://www.lmdb.tech/doc/, say, “Do not have open an LMDB database twice in the same process at the same time. Not even from a plain open() call - close()ing it breaks flock() advisory locking.” And database != environment in LMDB.

And one can open multiple (named) databases from a single environment. So I’ve been wondering if that second statement uses the word “database” colloquially, to refer to an LMDB environment; or if it’s actually the case that one shouldn’t open the same LMDB database (a.k.a. Rkv::Store) twice in the same process, in which case Rkv should enforce that constraint.

And he responded:

I believe both constraints apply. The DB one is most practically relevant.

So we may want manager.rs to manage databases too, and not just environments. But that isn't what it does today, and we should be clear about that. Hence this change.

The Rkv struct wraps an LMDB "environment".  The Store struct wraps an LMDB "database".  This change ensures we use the terms "environment" and "store" consistently in the Manager implementation.
@mykmelez mykmelez requested a review from ncalexan June 5, 2018 21:03
Copy link
Member

@ncalexan ncalexan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, this looks good.

If we really aren't supposed to open an LMDB database (== Rkv store) more than once in a process, will the manager grow to hand out Arc<Store> and ensure that the Store itself is Sync?

@mykmelez
Copy link
Contributor Author

mykmelez commented Jun 5, 2018

If we really aren't supposed to open an LMDB database (== Rkv store) more than once in a process, will the manager grow to hand out Arc and ensure that the Store itself is Sync?

I think so, yes. @rnewman should chime in if he was thinking otherwise!

@mykmelez
Copy link
Contributor Author

mykmelez commented Jun 6, 2018

If we really aren't supposed to open an LMDB database (== Rkv store) more than once in a process, will the manager grow to hand out Arc and ensure that the Store itself is Sync?

I think so, yes. @rnewman should chime in if he was thinking otherwise!

@ncalexan I filed this as #29.

@mykmelez mykmelez deleted the use-consistent-terminology branch June 6, 2018 14:42
@rnewman
Copy link
Contributor

rnewman commented Jun 7, 2018

The LMDB database handle is Copy; that is, one doesn't need to pass around Arc. The restriction is that a file path is only opened once, not that only one instance of the handle exists in memory. Now if Store is heavyweight, or you need a good place to handle close semantics, then you might want borrowing, but it seems to be easier to pass by value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants