Don't take archived log size into account when calculating log size for flush #12680
Conversation
HypenZou
force-pushed
the
checkpoint_log_flush
branch
3 times, most recently
from
67d70de
to
e975dd1
Compare
|
@hx235 thx for review🙏🏻 |
HypenZou
force-pushed
the
checkpoint_log_flush
branch
4 times, most recently
from
8b4824c
to
e284e38
Compare
|
@ajkr has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
| ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->SetCallBack( | ||
| "DBImpl::FlushForGetLiveFiles", [&](void*) { flushed = true; }); |
There was a problem hiding this comment.
The use of SyncPoint here is a little excessive, but OK. One downside of SyncPoint is that it's error-prone - the string name could have typo and that could make the test accidentally pass. Another downside is it's low-level - there could be a different code path flushing the memtable.
The more conventional alternative would be to check NumTableFilesAtLevel(0) is the same before and after the checkpoint. You could even make that a little bit more robust by setting disable_auto_compactions=true to prevent compaction out of L0.
There was a problem hiding this comment.
thx for advice, updated it
|
@HypenZou has updated the pull request. You must reimport the pull request before landing. |
|
@ajkr has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
HypenZou
force-pushed
the
checkpoint_log_flush
branch
from
daa2769
to
c5f502a
Compare
|
@HypenZou has updated the pull request. You must reimport the pull request before landing. |
HypenZou
force-pushed
the
checkpoint_log_flush
branch
from
c5f502a
to
1102c1d
Compare
|
@HypenZou has updated the pull request. You must reimport the pull request before landing. |
|
@ajkr has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Context/Summary:
It seems unreasonable to take the archived log size into account when calculating log size for flush in method CreateCheckpoint. If the user sets WAL_ttl_seconds or WAL_size_limit_MB, the argument log_size_for_flush can easily be reached due to the size of the archived dir. As a result, the flush may always be triggered.
Test
corverd by ./checkpoint_test