Use SST file manager to track blob files as well #8037

akankshamahajan15 · 2021-03-06T03:00:34Z

Summary: Extend support to track blob files in SST File manager.
This PR notifies SstFileManager whenever a new blob file is created,
via OnAddFile and an obsolete blob file deleted via OnDeleteFile
and delete file via ScheduleFileDeletion.

Test Plan: Add new unit tests

Reviewers:

Subscribers:

Tasks: T85239542

Tags:

facebook-github-bot

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

facebook-github-bot · 2021-03-09T00:20:45Z

@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot

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

facebook-github-bot · 2021-03-09T01:24:59Z

@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-03-09T01:27:36Z

@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-03-09T01:42:43Z

@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot

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

db/builder.cc

db/compaction/compaction_job.cc

ltamasi

Thanks for the PR @akankshamahajan15 !

db/blob/db_blob_compaction_test.cc

db/builder.cc

db/compaction/compaction_job.cc

db/db_impl/db_impl_open.cc

db/db_test_util.cc

db/compaction/compaction_job.cc

ltamasi · 2021-03-10T22:27:07Z

@akankshamahajan15
Sorry, couple more things:

Could you also update the comments in sst_file_manager_impl.{h,cc} as needed?
The two OnAddFile overloads and OnAddFileImpl have a boolean parameter compaction, which, as far as I can tell, is always false. Could you confirm this? If so, we could remove this flag and clean up all the code and variables that are related to it. (For instance, seems to me that in_progress_files_size_ / in_progress_files_ are redundant, assuming that compaction is indeed always false.)

akankshamahajan15 · 2021-03-10T22:49:48Z

The two OnAddFile overloads and OnAddFileImpl have a boolean parameter compaction, which, as far as I can tell, is always false. Could you confirm this? If so, we could remove this flag and clean up all the code and variables that are related to it. (For instance, seems to me that in_progress_files_size_ / in_progress_files_ are redundant, assuming that compaction is indeed always false.)

Yes, you are right. I checked all the calls and compaction is always false. So I will update the code and remove this parameter and its related code.

facebook-github-bot · 2021-03-15T23:23:47Z

@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-03-15T23:31:05Z

@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing.

riversand963

Took another pass, thanks @akankshamahajan15 for the updates.

HISTORY.md

db/blob/blob_file_completion_callback.h

db/compaction/compaction_job.cc

facebook-github-bot · 2021-03-16T17:23:01Z

@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing.

ltamasi

Thanks @akankshamahajan15 ! Looks really nice in general, just some minor comments.

db/builder.cc

db/blob/blob_file_completion_callback.h

db/db_impl/db_impl_open.cc

file/sst_file_manager_impl.cc

ltamasi · 2021-03-16T18:50:53Z

One final thing: as we discussed, failed flushes immediately clean up after themselves here: https://github.com/facebook/rocksdb/blob/master/db/builder.cc#L330-L346 .

Now, when this piece of logic is triggered, we haven't yet notified sfm of the SST; however, we have notified it of any blob files that might have been created. So here, we would need to call OnDeleteFile for the blob files in blob_file_paths as we iterate. (If the deletion was successful that is, which would also enable us to get rid of the PermitUncheckedError there.)

P.S. This could be done through another method in the callback class.

facebook-github-bot · 2021-03-17T00:47:51Z

@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-03-17T01:42:43Z

@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot

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

ltamasi

Awesome, thanks once again @akankshamahajan15 !

db/blob/blob_file_completion_callback.h

db/db_impl/db_impl.cc

facebook-github-bot · 2021-03-17T22:28:47Z

@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing.

Summary: Extend support to track blob files in SST File manager. This PR notifies SstFileManager whenever a new blob file is created, via OnAddFile and an obsolete blob file deleted via OnDeleteFile and delete file via ScheduleFileDeletion. Test Plan: Add new unit tests Reviewers: Subscribers: Tasks: Tags:

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

…s in SSTFileManager Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

Summary: Add a interface BlobFileCompletionCallback that is passed to BlobFileBuilder and when a blob file is closed, the interface notifies SSTFileManager about the file. Test Plan: Reviewers: Subscribers: Tasks: Tags:

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

facebook-github-bot · 2021-03-17T22:29:12Z

@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot

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

facebook-github-bot · 2021-03-18T03:45:01Z

@akankshamahajan15 merged this pull request in 27d57a0.

Summary: Extend support to track blob files in SST File manager. This PR notifies SstFileManager whenever a new blob file is created, via OnAddFile and an obsolete blob file deleted via OnDeleteFile and delete file via ScheduleFileDeletion. Pull Request resolved: facebook/rocksdb#8037 Test Plan: Add new unit tests Reviewed By: ltamasi Differential Revision: D26891237 Pulled By: akankshamahajan15 fbshipit-source-id: 04c69ccfda2a73782fd5c51982dae58dd11979b6 Signed-off-by: Changlong Chen <levisonchen@live.cn>

facebook-github-bot added the CLA Signed label Mar 6, 2021

facebook-github-bot reviewed Mar 8, 2021

View reviewed changes

akankshamahajan15 requested a review from ltamasi March 8, 2021 19:43

akankshamahajan15 force-pushed the blobdb_sstmanager branch from 17fafd4 to 9d7a4c3 Compare March 9, 2021 00:20

facebook-github-bot reviewed Mar 9, 2021

View reviewed changes

akankshamahajan15 force-pushed the blobdb_sstmanager branch from 9d7a4c3 to 37d20d9 Compare March 9, 2021 01:24

akankshamahajan15 force-pushed the blobdb_sstmanager branch from 37d20d9 to 0e1e896 Compare March 9, 2021 01:27

akankshamahajan15 force-pushed the blobdb_sstmanager branch from 0e1e896 to 158fe2a Compare March 9, 2021 01:42

facebook-github-bot reviewed Mar 9, 2021

View reviewed changes

riversand963 reviewed Mar 10, 2021

View reviewed changes

db/builder.cc Outdated Show resolved Hide resolved

riversand963 reviewed Mar 10, 2021

View reviewed changes

db/compaction/compaction_job.cc Outdated Show resolved Hide resolved

ltamasi reviewed Mar 10, 2021

View reviewed changes

db/blob/db_blob_compaction_test.cc Show resolved Hide resolved

db/builder.cc Outdated Show resolved Hide resolved

db/compaction/compaction_job.cc Outdated Show resolved Hide resolved

db/db_impl/db_impl_open.cc Show resolved Hide resolved

db/db_test_util.cc Outdated Show resolved Hide resolved

ltamasi reviewed Mar 10, 2021

View reviewed changes

db/db_test_util.cc Show resolved Hide resolved

ltamasi reviewed Mar 10, 2021

View reviewed changes

db/compaction/compaction_job.cc Outdated Show resolved Hide resolved

akankshamahajan15 force-pushed the blobdb_sstmanager branch from 158fe2a to 68c319d Compare March 15, 2021 23:23

akankshamahajan15 force-pushed the blobdb_sstmanager branch from 68c319d to b505e08 Compare March 15, 2021 23:31

riversand963 reviewed Mar 15, 2021

View reviewed changes

HISTORY.md Outdated Show resolved Hide resolved

db/blob/blob_file_completion_callback.h Outdated Show resolved Hide resolved

db/compaction/compaction_job.cc Outdated Show resolved Hide resolved

akankshamahajan15 force-pushed the blobdb_sstmanager branch from b505e08 to f676622 Compare March 16, 2021 17:22

ltamasi reviewed Mar 16, 2021

View reviewed changes

akankshamahajan15 force-pushed the blobdb_sstmanager branch from f676622 to 8c35f69 Compare March 17, 2021 00:47

facebook-github-bot reviewed Mar 17, 2021

View reviewed changes

ltamasi approved these changes Mar 17, 2021

View reviewed changes

db/blob/blob_file_completion_callback.h Outdated Show resolved Hide resolved

db/db_impl/db_impl.cc Show resolved Hide resolved

akankshamahajan15 force-pushed the blobdb_sstmanager branch from b42b58b to c2b7a2d Compare March 17, 2021 22:28

akankshamahajan15 added 10 commits March 17, 2021 15:28

Add new unit tests

c1ef04c

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

Update comments and History to include blob files in space/size limit…

308b061

…s in SSTFileManager Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

Fix LITE build

ab8fbe7

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

Fix LINTER error

d39226c

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

Addressed comments

38cfd59

Summary: Add a interface BlobFileCompletionCallback that is passed to BlobFileBuilder and when a blob file is closed, the interface notifies SSTFileManager about the file. Test Plan: Reviewers: Subscribers: Tasks: Tags:

Remove dead code from SstFileManager

eae8843

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

Minor changes

0178dd5

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

Fix ASSERT_STATUS_CHECKED failure

3f4596a

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

Minor changes

2ccbc1c

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

akankshamahajan15 force-pushed the blobdb_sstmanager branch from c2b7a2d to 2ccbc1c Compare March 17, 2021 22:29

facebook-github-bot reviewed Mar 17, 2021

View reviewed changes

facebook-github-bot closed this in 27d57a0 Mar 18, 2021

facebook-github-bot added the Merged label Mar 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use SST file manager to track blob files as well #8037

Use SST file manager to track blob files as well #8037

akankshamahajan15 commented Mar 6, 2021

facebook-github-bot left a comment

facebook-github-bot commented Mar 9, 2021

facebook-github-bot left a comment

facebook-github-bot commented Mar 9, 2021

facebook-github-bot commented Mar 9, 2021

facebook-github-bot commented Mar 9, 2021

facebook-github-bot left a comment

ltamasi left a comment

ltamasi commented Mar 10, 2021 •

edited

Loading

akankshamahajan15 commented Mar 10, 2021 •

edited

Loading

facebook-github-bot commented Mar 15, 2021

facebook-github-bot commented Mar 15, 2021

riversand963 left a comment

facebook-github-bot commented Mar 16, 2021

ltamasi left a comment

ltamasi commented Mar 16, 2021 •

edited

Loading

facebook-github-bot commented Mar 17, 2021

facebook-github-bot commented Mar 17, 2021

facebook-github-bot left a comment

ltamasi left a comment

facebook-github-bot commented Mar 17, 2021

facebook-github-bot commented Mar 17, 2021

facebook-github-bot left a comment

facebook-github-bot commented Mar 18, 2021

Use SST file manager to track blob files as well #8037

Use SST file manager to track blob files as well #8037

Conversation

akankshamahajan15 commented Mar 6, 2021

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Mar 9, 2021

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Mar 9, 2021

facebook-github-bot commented Mar 9, 2021

facebook-github-bot commented Mar 9, 2021

facebook-github-bot left a comment

Choose a reason for hiding this comment

ltamasi left a comment

Choose a reason for hiding this comment

ltamasi commented Mar 10, 2021 • edited Loading

akankshamahajan15 commented Mar 10, 2021 • edited Loading

facebook-github-bot commented Mar 15, 2021

facebook-github-bot commented Mar 15, 2021

riversand963 left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Mar 16, 2021

ltamasi left a comment

Choose a reason for hiding this comment

ltamasi commented Mar 16, 2021 • edited Loading

facebook-github-bot commented Mar 17, 2021

facebook-github-bot commented Mar 17, 2021

facebook-github-bot left a comment

Choose a reason for hiding this comment

ltamasi left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Mar 17, 2021

facebook-github-bot commented Mar 17, 2021

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Mar 18, 2021

ltamasi commented Mar 10, 2021 •

edited

Loading

akankshamahajan15 commented Mar 10, 2021 •

edited

Loading

ltamasi commented Mar 16, 2021 •

edited

Loading