Mv level work3 ... change from 3 overlapped levels to 2 #84

Merged
merged 5 commits into from Jun 26, 2013

Conversation

Projects
None yet
3 participants
Contributor

matthewvon commented Jun 25, 2013

Details here: https://github.com/basho/leveldb/wiki/Mv-level-work3

Three overlapped levels gives us better performance, especially in write heavy and in read recent heavy environments. However, the conversion time to shift 1.3 level files into 1.4 formation would have taken hours on big installations. So only doing one extra overlapped this release. It will also smooth the way for sorter upgrade from 2 to 3 in a future release.

MatthewVon added some commits Jun 23, 2013

MatthewVon clear 3 compiler warnings (unsigned vs signed). go to 2 overlapped le…
…vels from 3 (upgrade issue). adjust VersionSet::Finalize() penalty values again based upon testing.
74d5f82
Matthew V Add routine to pause DBImpl::Recover while new overlapped areas are c…
…leared via compactions (Riak 1.3 to 1.4 conversion). Also cleaned some int versus size_t compiler warnings.
9a8d9a0
MatthewVon move throttle and reduce mutex_ lock calls by one. change factor in F…
…inalize() back to 8, was 2 recently (2i_slf_stressmix test)
0eaa716
MatthewVon perf top indicates the pthread timed wait was heavy hitter. reduced n…
…umber of times a compaction thread call PrioritizeWork() to reduce unnecessary overhead. compactions are running too slow in 2i_slf_stressmix
25000de

matthewvon was assigned Jun 25, 2013

I have not been able to find any issues with the code beyond that CorruptionTest.TableFileIndexData test failure. It would be great to fix that so the test suite passes cleanly. Don't forget to create an issue to test a change to handle the throttle skipping path in DBImpl::Write. With those caveats, 👍

MatthewVon Move the DB::Open pause code to new location so as to let foreground …
…processing of recovery logs properly post to version_set before starting background compactions.
206b277
Contributor

jtuple commented Jun 26, 2013

In addition to looking at the code, ended up testing the code in a few scenarios. One scenario was writing data using LevelDB from Riak 1.3.2 and then upgrading the node to this branch of LevelDB. The goal was to trigger the overlap compaction logic.

In preliminary testing earlier today, I was able to hit this case but then ran into repeated errors in the LOG of the form:

2013/06/25-15:13:47.600403 b4502000 Compaction error: IO error: ./data/leveldb/0/sst_0/000514.sst: No such file or directory
2013/06/25-15:13:47.600424 b4502000 Waiting after background compaction error: IO error: ./data/leveldb/0/sst_0/000514.sst: No such file or directory

This issue has successfully been resolved in the latest commit 206b277. Specifically, despite being able to repeatedly reproduce the issue prior to this commit, I cannot reproduce it after. Furthermore, the logic in the commit makes complete sense to me: ie. delaying the forced compaction logic until after the recovery code has run. Also, since the call to CheckCompactionState occurs where LevelDB previously called MaybeScheduledCompaction it follows that any pre-conditions necessary for compaction to be safely called must already be met.

One thought that crossed my mind would be to add something to the LOG output that notes the number of overlapping levels being used, similar to how to print out the write buffer / bloom filter version / etc to the LOG. The more information we have in the LOGs the better. In any case, this is just a suggestion and is future work at that.

Note for historical purposes: issue #85 arose out of internal discussion on this pull-request. It is future work to keep the code change here minimal and move along with current release plans.

+1 merge

@jtuple jtuple added a commit that referenced this pull request Jun 26, 2013

@jtuple jtuple Merge pull request #84 from basho/mv-level-work3 31fa6dd

@jtuple jtuple merged commit 31fa6dd into master Jun 26, 2013

matthewvon deleted the mv-level-work3 branch May 5, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment