Reactivate Google's PickLevelForMemTableOutput()

[matthewvon]

This routine used to "be in the way" when efforts focused on random data ingest. Now, it has new relevance as Riak's handoff and other sequential features come into play.

Detail discussion is here:

https://github.com/basho/leveldb/wiki/mv-initial-level-fix

[paulplace]

The builder added the file to table_cache with mutex_ unlocked; is there any possibility that another thread could have referenced the file via the table_cache before we get to this point?

[matthewvon]

The new file is not added to the manifest until after this routine returns. The “edit” parameter gets set at line 767 with the new file number and its level. No thread can access this file in the table cache because the file number is not known outside this routine. The “edit” object subsequently gets posted to the manifest. That is when the file number becomes visible.

On Aug 8, 2015, at 4:06 PM, Paul A. Place <notifications@github.com mailto:notifications@github.com> wrote:

In db/db_impl.cc #163 (comment):

         std::string old_name, new_name;

         old_name=TableFileName(options_, meta.number, 0);
         new_name=TableFileName(options_, meta.number, level);

•        s=env_->RenameFile(old_name, new_name);
  

•        move_s=env_->RenameFile(old_name, new_name);
  

•        if (move_s.ok())
  

•        {
  

•            // builder already added file to table_cache with 2 references and
  

  The builder added the file to table_cache with mutex_ unlocked; is there any possibility that another thread could have referenced the file via the table_cache before we get to this point?

—
Reply to this email directly or view it on GitHub https://github.com/basho/leveldb/pull/163/files#r36581285.

[paulplace]

Notes to Matthew (github wouldn't let me add these comments inline, since they refer to parts of the code that were not modified):

• in db/db_impl.cc, line 1866: DBImpl::BuildBatchGroup() assumes mutex_ is locked; should probably add a comment and "mutex_.AssertHeld();" statement like we have with DBImpl::MakeRoomForWrite()
• db/version_set.cc, line 495: remember to fix signed/unsigned comparison warning (Matthew already caught this...just adding a reminder)

[matthewvon]

db/db_impl.cc line 1866: updated as suggested

db/version_set.cc line 495 corrected: now uses uint64_t instead of int64_t.

On Aug 8, 2015, at 4:14 PM, Paul A. Place notifications@github.com wrote:

Notes to Matthew (github wouldn't let me add these comments inline, since they refer to parts of the code that were not modified):

in db/db_impl.cc, line 1866: DBImpl::BuildBatchGroup() assumes mutex_ is locked; should probably add a comment and "mutex_.AssertHeld();" statement like we have with DBImpl::MakeRoomForWrite()

db/version_set.cc, line 495: remember to fix signed/unsigned comparison warning (Matthew already caught this...just adding a reminder)

—
Reply to this email directly or view it on GitHub #163 (comment).

[paulplace]

+1 code review looks good