Jul27

Hi Brian,

This change has two logical parts.

**Part 1:** I thought of it and came to the conclusion that Stream-level code should be the one dealing with storage key prefixes, while Storage-level code should just work on the keys being plain strings.

The justification is simple: If Storage-level code has to support key prefixes, it would effectively have to parse each key before making the decision on whether the iterator is already Done(). This is due to the fact that two conceptually checks have to be performed: one check for prefix match, and another check for primary key order.

As I remember, this matches our conversation in person end of last week. Hope I remember it well :-)

**Part 2:** I have promoted LevelDB-based storage and iterators to first-class citizens.

This allows running both Storage-level and Stream-level tests on both mock, in-memory Storage implementation, and on the LevelDB one.

The LevelDB implementation had to be wrapped into a helper class that would physically remove all LevelDB files between creations and destruction of the Storage object, since each of them has to be independent.

It also involved fixing some logic in the test code: apparently, one pair of Storage+Stream managers has been created within the lifetime of another one. With the same directory used for test LevelDB instances, this resulted in a non-trivial error. Fixed now.

Looking back, your design of LevelDB iterators being wrapped into more generic ones might be an overkill. As I understand, it used to involve multiple smart pointers for resource control (quick I have simplified a bit into a unique_ptr-s and a pure reference). I had to go quite some way around to have LevelDB storage expose the same (simple!) interface as the in-memory one does.

Many diff lines in this pull request are file renames. The change itself is rather small though.

Please review it. Looking forward to syncing up in person soon.

Thank you,
Dima
LGTM


On Tue, Jul 29, 2014 at 8:32 AM, Dima <notifications@github.com> wrote:

> In src/dbm_leveldb.cc:
>
> > -    if (!s.ok()) throw std::domain_error(s.ToString());
> > -    ::TailProduce::Storage::VALUE_TYPE v_ret(v_get.begin(), v_get.end());
> > -    return v_ret;
> > +    if (s.IsNotFound()) {
> > +        throw ::TailProduce::StorageNoDataException();
> > +    } else {
> > +        if (!s.ok()) throw std::domain_error(s.ToString());
> > +        ::TailProduce::Storage::VALUE_TYPE v_ret(v_get.begin(), v_get.end());
> > +        return v_ret;
> > +    }
> > +}
> > +
> > +bool TailProduce::DbMLevelDb::HasRecord(::TailProduce::Storage::KEY_TYPE const& key) {
> > +    std::string v_get;
> > +    leveldb::Status s = db_->Get(leveldb::ReadOptions(), key, &v_get);
> > +    return !s.IsNotFound();
>
> Also, it's only LevelDB that makes this operation expensive. I would
> expect many/most other storage to provide some means to invoke Has() much
> faster than Get().
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/Staance/tailproduce/pull/22/files#r15530676>.
>