Split BlobFileState into an immutable and a mutable part #6502
Conversation
There was a problem hiding this comment.
@ltamasi has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
| Status BlobFileState::DecodeFrom(Slice* input) { | ||
| constexpr char class_name[] = "BlobFileState"; | ||
| Status BlobFileAddition::DecodeFrom(Slice* input) { | ||
| constexpr char class_name[] = "BlobFileAddition"; |
There was a problem hiding this comment.
Wonder whether this can be class static.
There was a problem hiding this comment.
Could be done but for now, it's only used in a single method so I think it would make sense to keep it there.
| }); | ||
| SyncPoint::GetInstance()->EnableProcessing(); | ||
|
|
||
| constexpr uint64_t blob_file_number = 678; |
There was a problem hiding this comment.
Nit: maybe rename constants to kBlobFileNumber or something similar?
There was a problem hiding this comment.
Not a huge fan of the Google C++ style guide :) but fortunately, for local variables this naming scheme is optional (see https://google.github.io/styleguide/cppguide.html#Constant_Names).
|
Thanks for the review @riversand963 ! |
|
@ltamasi has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
@ltamasi has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Summary:
It's never too soon to refactor something. The patch splits the recently
introduced (
VersionEditrelated)BlobFileStateinto two classesBlobFileAdditionandBlobFileGarbage. The idea is that once blob filesare closed, they are immutable, and the only thing that changes is the
amount of garbage in them. In the new design,
BlobFileAdditioncontainsthe immutable attributes (currently, the count and total size of all blobs, checksum
method, and checksum value), while
BlobFileGarbagecontains the mutableGC-related information elements (count and total size of garbage blobs). This is a
better fit for the GC logic and is more consistent with how SST files are handled.
Test Plan:
make check