Blob DB: Improve FIFO eviction

[yiwu-arbug]

Summary:
Improving blob db FIFO eviction with the following changes,

• Change blob_dir_size to max_db_size. Take into account SST file size when computing DB size.
• FIFO now only take into account live sst files and live blob files. It is normal for disk usage to go over max_db_size because there are obsolete sst files and blob files pending deletion.
• FIFO eviction now also evict TTL blob files that's still open. It doesn't evict non-TTL blob files.
• If FIFO is triggered, it will pass an expiration and the current sequence number to compaction filter. Compaction filter will then filter inlined keys to evict those with an earlier expiration and smaller sequence number. So call LSM FIFO.
• Compaction filter also filter those blob indexes where corresponding blob file is gone.
• Add an event listener to listen compaction/flush event and update sst file size.
• Implement DB::Close() to make sure base db, as well as event listener and compaction filter, destruct before blob db.
• More blob db statistics around FIFO.
• Fix some locking issue when accessing a blob file.

Test Plan:
See the new tests.

[facebook-github-bot]

@yiwu-arbug has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[facebook-github-bot]

@yiwu-arbug has updated the pull request. View: changes, changes since last import

[yuslepukhin]

@anand1976 I think we have a problem now with Close() still being virtual and people override that instead of CloseImpl(). This will make a confusing experience with some DB overriding Close() and others CloseImpl(). Should Close() be made non-virtual? What was the final decision?

[ajkr]

Yeah I agree it should be non-virtual.. if people override Close() they can't set closed_ = true and then DBImpl::CloseHelper will always be called in destruction.

[ajkr]

Oh ignore my comment. This StackableDB class doesn't subclass DBImpl so my comment was wrong.

[yiwu-arbug]

Discussed with @anand1976 and @ajkr. I'm going to implement CloseImpl() not Close(). I'll split the change to another PR for review.

[anand1976]

There is no DB::CloseImpl(). The Close()/CloseImpl() pattern was only for the Logger class. Although after talking with @yiwu-arbug and @ajkr it makes sense to have the same pattern in DB as well.

[yiwu-arbug]

I'll go ahead with my current PR and wait till changes on DBImpl side to change to use CloseImpl().

[facebook-github-bot]

@yiwu-arbug has updated the pull request. View: changes, changes since last import

[facebook-github-bot]

@yiwu-arbug has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[facebook-github-bot]

@yiwu-arbug has updated the pull request.

[facebook-github-bot]

@yiwu-arbug has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[sagar0]

This looks good to me from the BlobDB perspective, subject to the discussion around close/closeimpl. Lets update here with the summary of the discussion that you had with Anand about Close.

[yiwu-arbug]

Regarding DB::Close(), @anand1976 will make the change to DBImpl similar to what he did for Logger #3528. Subclasses of StackableDB will need to implement a ::CloseImpl method and the order of execution will be analogous to destructor: derived class's CloseImpl will be called first and then base class's CloseImpl. StackableDB::Close() will be non-virtual and is responsible to call CloseImpl in order, and setting closed_ flag so a second Close() will be noop. I'll leave the implementation here as-is, and wait till the change on DBImpl side and make change to BlobDB accordingly.

[facebook-github-bot]

@yiwu-arbug is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.