Add io_buffer_size to BackupEngineOptions #13236
Conversation
archang19
force-pushed
the
backup-engine-io-buffer-size
branch
6 times, most recently
from
1316c31 to
82348e9
Compare
archang19
force-pushed
the
backup-engine-io-buffer-size
branch
2 times, most recently
from
147b60f to
ac0178c
Compare
|
@archang19 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
| : kDefaultCopyFileBufferSize; | ||
| size_t buf_size = CalculateIOBufferSize(rate_limiter); | ||
| TEST_SYNC_POINT_CALLBACK( | ||
| "BackupEngineImpl::CopyOrCreateFile:CalculateIOBufferSize", &buf_size); |
There was a problem hiding this comment.
Sync point is not my favorite here for testing about 1/2 to 2/3rds of the integrated data path of the setting when an existing callback mechanism (FileSystemWrapper / FSWritableFile / FSSequentialFile) could test the full end-to-end buffer size (probably tracking the max buffer size seen in existing TestFs in backup_engine_test). But it's ok.
archang19
force-pushed
the
backup-engine-io-buffer-size
branch
from
a0e7bf6 to
fcc26da
Compare
|
@archang19 has updated the pull request. You must reimport the pull request before landing. |
|
@archang19 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@archang19 has updated the pull request. You must reimport the pull request before landing. |
|
@archang19 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
archang19
force-pushed
the
backup-engine-io-buffer-size
branch
from
1bfc7d7 to
5df6b7e
Compare
|
@archang19 has updated the pull request. You must reimport the pull request before landing. |
|
@archang19 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@archang19 merged this pull request in f7d32c1. |
Summary
The RocksDB backup engine code currently derives the IO buffer size based on the following criteria:
We want to be able to explicitly choose the IO size based on the storage backend. We want the new criteria to be:
BackupEngineOptionsThis PR adds a new option called
io_buffer_sizetoBackupEngineOptionsand updates the logic used to set the buffer size.Test Plan
I added a separate unit test and verified that we can either use the
io_buffer_size, rate limiter burst size, or the default size.I decided to use a
TEST_SYNC_POINT_CALLBACK. I considered the alternative of updating theReadimplementation ofDummySequentialFile/CheckIOOptsSequentialFileto check the value ofn. However, that would have considerably complicated the whole test code, and we also do not need to be checking for this in every single test case. I think theTEST_SYNC_POINT_CALLBACKturned out to be quite elegant.