limit leveldb synctree lock to the local node

[ian-mi]

As the intention of this use of global:trans appears to be to lock access to a synctree locally, limit the lock only on the local node. Hopefully this will help to reduce ensemble startup time. Attempts to address issue #70 (RIAK-1831).

[ian-mi]

As Andrew Stone noted in this comment locking can be avoided entirely if we check the result of ets:insert_new. I have update my pull request to do so.

[engelsanchez]

If safe_open returns {ok, DB}, you are almost guaranteed to have the one true instance open on that path (except for extreme timing races), and whatever is stored in the ETS is likely stale.

[nickelization]

So, just to make sure I'm understanding this, eleveldb:open cannot open the same path twice at the same time, right? There's some sort of locking mechanism or something in there? I'm just assuming that's the way it works based on our discussions earlier and the way you've written the code, but I don't actually know myself. If that is indeed how it works, then everything looks great.

[nickelization]

Also, it looks like the safe_open and reopen functions are no longer called by anything (reopen is exported, but I don't see it being used anywhere, and since this is fairly deep in the internals of riak_ensemble I assume it's not meant to be a public API function or anything). Not sure if we should consider removing those functions now, or maybe we want to leave them in for possible future use; I could see arguments either way.

[engelsanchez]

@nickelization Yes, eleveldb has an internal lock that should prevent multiple instances opening the same directory. We should rely on that guarantee here.

[ian-mi]

I agree that reopen and safe_open should be removed as they should probably not be used instead of maybe_open_leveldb.

[nickelization]

👍 7744c34

[cmeiklejohn]

Am I missing something here? When retries gets to 0, won't this code just generate a case_clause exception?

[cmeiklejohn]

Ah, whoops. It looks like that was the behavior before as well.

[ian-mi]

I decided to keep the same behavior and generate the case_clause exception. I could easily change the code to pass the last error to the caller and match the return value, changing the generated exception to a bad_match exception, if you think that would be preferable.

[nickelization]

Since it seems to fix the problem in question, and otherwise has the same behavior as before, I'm going to go ahead and merge this in since we're in a rush to get this out to a customer. If anyone else who's been reviewing this notices anything that's been missed let me know, but since it looks like Engel, Chris, and myself have all reviewed now I'm going to assume we're good here.

[nickelization]

@borshop merge