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

Add 2I to Memory backend #314

Merged
merged 9 commits into from
Apr 12, 2012
Merged

Add 2I to Memory backend #314

merged 9 commits into from
Apr 12, 2012

Conversation

seancribbs
Copy link
Contributor

As it says, this adds secondary indexes to the memory backend. Why?

  • The memory backend is useful when running automated test suites because it is less expensive than creating and deleting LevelDB instances. (A special "test" mode has also been added, with a corresponding reset() function.)
  • ETS is capable of providing this feature with minimal additional work.

Caveats:

  • Index postings are not considered when computing the size of an object, and therefore do not count towards its eviction when used as an LRU. This could be added later but is potentially expensive to compute without introducing a coupling to riak_object.
  • It is necessary to use ets:match_delete/2 and ets:select/1,3 operations to efficiently select items from the index, as ETS has no concept of cursors beyond ets:first/1 and ets:next/2 which do not let you specify a starting key. match and select have been known to be locky in certain scenarios. ets:match_delete/2 must be used when deleting all postings for a bucket/key from the index table, due to the natural ordering of index keys. This penalty is only incurred when removing objects in LRU mode.

@ghost ghost assigned seancribbs Apr 10, 2012
@slfritchie
Copy link
Contributor

I'm a bit puzzled by the comment about ets:next/2, because that func can give you cursor-like traversal. But if you've got an implementation using match_delete and select that works, I don't have anything to complain about. "Locky" is a term of art for "don't expect this to perform well". :-)

@seancribbs
Copy link
Contributor Author

@slfritchie "which do not let you specify a starting key" is the main point there, sorry. In contrast, LevelDB lets you seek to a starting key, then iterate. How would you do the same in ETS?

@slfritchie
Copy link
Contributor

With an ordered_set table, ets:next(Tab, AnyKeyOfYourChoosing) should choose the next key. For other types, you need to use safe_fixtable() before getting deterministic results from next/2. IIRC.

@seancribbs
Copy link
Contributor Author

A few tidbits from the Efficiency Guide:

Select/Match operations on Ets and Mnesia tables can become very expensive operations. They usually need to scan the complete table. You should try to structure your data so that you minimize the need for select/match operations.

...

There are exceptions when the complete table is not scanned, for instance if part of the key is bound when searching an ordered_set table, or if it is a Mnesia table and there is a secondary index on the field that is selected/matched.

In this case, because a large portion of the key is bound in the match specs, it should be less likely to need a full-table scan.

@seancribbs
Copy link
Contributor Author

@slfritchie Aha, so it could be done with next/2 (the table is an ordered_set). Is it worth rewriting it?

@slfritchie
Copy link
Contributor

If this new feature is only being used for testing, and it's not buggy, then I'd leave it as-is. If the new feature is meant for general use (including by customers), then it's worth getting rid of as many match_delete() and select() calls as possible: they're both too heavy-handed in an SMP world to use except when nothing else suffices.

@seancribbs
Copy link
Contributor Author

@slfritchie Thanks. It might be difficult to remove match_delete(), but removing select() is do-able. Although this is useful to testing environments, some will undoubtedly use the memory backend in production. Better to head this one off at the pass.

@seancribbs
Copy link
Contributor Author

@slfritchie Ok, I've switched as much as possible to use lookup() and next() over select(). Can you give it a good thrashing?

@slfritchie
Copy link
Contributor

Sean, things look pretty sane. If the Dialyzer isn't angered more by these patches, then +1 -- ideally, the module would be warning-free.

@seancribbs
Copy link
Contributor Author

Ok, I will dialyze and fix.

Sean Cribbs added 2 commits April 12, 2012 16:04
* CRITICAL: riak_kv_vnode should cons the async_folds option onto the
  backend configuration, not nest the latter in a new list.
* The key_range_folder doesn't work properly for entire bucket lists,
  instead use the regular folder.
* Simplify further the implementation of reset/0,2.
@seancribbs
Copy link
Contributor Author

@slfritchie One final look if you will? Thank you so much for your help on this.

@slfritchie
Copy link
Contributor

+1 to merge, many thanks!

seancribbs pushed a commit that referenced this pull request Apr 12, 2012
@seancribbs seancribbs merged commit 49be5a6 into master Apr 12, 2012
seancribbs pushed a commit to basho/riak-ruby-client that referenced this pull request Apr 12, 2012
@seancribbs seancribbs deleted the t27-2i-memory-backend branch April 1, 2015 23:18
@seancribbs seancribbs removed their assignment May 8, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants