-
Notifications
You must be signed in to change notification settings - Fork 6.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove Deprecated overloads of DB::GetApproximateSizes #9458
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just one concern which does not need to be addressed in this PR.
If we remove these two overloads, a user code calling the removed GetApproximateSizes(..., bool include_memtable)
may not be caught by compiler. Instead, the include_memtable:bool
may be silently cast to include_flag:uint8_t
, leading to incorrect results. I think we should make SizeApproximationFlags
an enum class.
ef992ae
to
2c3e71f
Compare
@akankshamahajan15 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Summary: In RocksDB few overloads of DB::GetApproximateSizes are marked as DEPRECATED_FUNC, and we are removing it in the upcoming 7.0 release. Test Plan: CircleCI
2c3e71f
to
6cd9dc3
Compare
@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing. |
@akankshamahajan15 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing. |
@akankshamahajan15 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Summary: In RocksDB few overloads of DB::GetApproximateSizes are marked as
DEPRECATED_FUNC, and we are removing it in the upcoming 7.0 release.
Test Plan: CircleCI