make Manager consumable

[mykmelez]

@rnewman Manager::new would have to be pub in order for a consumer to use it to create a Manager instance that manages database handles for them. But perhaps you have a different plan for how consumers should use Manager?

I suppose it somewhat begs the question to enable consumers to instantiate them directly, since a consumer can then evade the constraint that Manager imposes by instantiating it multiple times. One alternative would be to create a Manager singleton via a lazy static, i.e. something like:

#[macro_use]
extern crate lazy_static;

lazy_static! {
    #[derive(Debug)]
    static ref MANAGER: Manager = {
        Manager::new()
    };
}

Then keep Manager::new() private to force consumers to get or create Rkv instances via the singleton.

[rnewman]

The idea was to not make the constructor public precisely to prevent any consumer from bypassing the single-instance restriction. If you’re brave enough to do that, just create stores directly rather than using Manager. There’s no benefit to having multiple managers.

[rnewman]

License block.

[mykmelez]

Fixed in 272c689.

[rnewman]

I don’t mind allowing Manager to be created for tests, but in that case please do so via a #[cfg(test)] pub(crate) method called new_for_test or something.

[mykmelez]

The idea was to not make the constructor public precisely to prevent any consumer from bypassing the single-instance restriction. If you’re brave enough to do that, just create stores directly rather than using Manager. There’s no benefit to having multiple managers.

Right, makes sense. However, in that case, how does one use Manager today? Perhaps I'm missing something, but there doesn't currently appear to be that single instance of Manager that consumers can access.

[rnewman]

There isn’t, and some kind of thread-safe singleton must be implemented. If you can make it eager/on module load, that’s fine; it doesn’t really need to be very lazy.

[mykmelez]

There isn’t, and some kind of thread-safe singleton must be implemented. If you can make it eager/on module load, that’s fine; it doesn’t really need to be very lazy.

Making it eager fails on "calls in statics are limited to struct and enum constructors," and while we could replace Manager::new() with a direct call to the struct constructor, we'd still need to call RwLock::new() to make it threadsafe.

(In the past, we would've been able to use StaticRwLock, but that was deprecated in favor of lazy_static in rust-lang/rust#27717.)

So 273ff58 uses lazy_static to add a thread-safe static singleton and makes Manager::new() private again.

[mykmelez]

@fluffyemily and @ncalexan: per our conversation earlier today about expanding the reviewer corps for rkv, perhaps y'all can look at this?

[ncalexan]

This tree is all over the place, with reversions and a merge from master. Land just one commit with no merge, please.

Otherwise, this looks fine, but I have a slight preference for exposing Manager::singleton() or Manager::instance() rather than the all-caps very-special lazy static to consumers. Lazy static instances have strange types, or at least they did.

In the diff from master, I noticed some tests were removed from src/env.rs. Explain why?

[mykmelez]

This tree is all over the place, with reversions and a merge from master. Land just one commit with no merge, please.

I have mixed feelings about this, as I sometimes appreciate the "messy" history of a feature's development when digging at code archaeological sites. But, ok, I'll "squash and merge" this pull request.

Otherwise, this looks fine, but I have a slight preference for exposing Manager::singleton() or Manager::instance() rather than the all-caps very-special lazy static to consumers. Lazy static instances have strange types, or at least they did.

They still do, as the macro generates a type that dereferences to the specified type of the static (which appears to be of type manager::MANAGER). It isn't clear to me how likely it is for consumers to be confused by this type. Nevertheless, I agree that Manager::singleton() seems more idiomatic and ergonomic, despite its added length.

In 6293a29, I've made MANAGER private and added a Manager::singleton() static method that dereferences MANAGER from its macro-generated type to RwLock<Manager> and then returns a reference to it.

(An alternative would be for Manager to implement RwLock, such that consumers can call Manager::read() and Manager::write() directly. That would simplify the API but perhaps also obfuscate it.)

In the diff from master, I noticed some tests were removed from src/env.rs. Explain why?

I didn't intend to do this, nor do I see it in this pull request, nor local diffs. Are you perhaps diffing locally against a local master branch that is behind upstream?

[mykmelez]

I have mixed feelings about this, as I sometimes appreciate the "messy" history of a feature's development when digging at code archaeological sites. But, ok, I'll "squash and merge" this pull request.

Erm, or did you mean to both squash all commits on this branch and also land it without a merge commit, fast-forwarding master to the squashed commit?

[ncalexan]

Erm, or did you mean to both squash all commits on this branch and also land it without a merge commit, fast-forwarding master to the squashed commit?

Squash so there's one commit; the false starts are rarely valuable. Rebase that onto master. Merge vs. fast-forward is your choice; I personally prefer merge with the r= on the merge commit, but GH makes that onerous.

[mykmelez]

Squash so there's one commit; the false starts are rarely valuable.

I'll agree to disagree. 😉

Rebase that onto master. Merge vs. fast-forward is your choice; I personally prefer merge with the r= on the merge commit, but GH makes that onerous.

Indeed, and it's quite useful to employ one of the flows that GitHub optimizes. In this case, it looks like "squash and merge" is the closest to what you prefer, so I'll do that.