-
Notifications
You must be signed in to change notification settings - Fork 9
Move ChecksumHandle and its methods into their own module
#472
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
Conversation
jorisdral
left a comment
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.
Looks good in general! I have some suggestions
| {- | $checksum-handles | ||
| A handle ('ChecksumHandle') that maintains a running CRC32 checksum. | ||
| -} |
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.
| {- | $checksum-handles | |
| A handle ('ChecksumHandle') that maintains a running CRC32 checksum. | |
| -} | |
| {- $checksum-handles | |
| A handle ('ChecksumHandle') that maintains a running CRC32 checksum. | |
| -} |
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.
I'm merely following the syntax of the block right above this one, so I think either zero or two changes are warranted.
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.
Which block do you mean?
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.
This one:
lsm-tree/src/Database/LSMTree/Internal/CRC32C.hs
Lines 241 to 244 in d0ae185
| {- | $checksum-files | |
| We use @.checksum@ files to help verify the integrity of on disk snapshots. | |
| Each .checksum file lists the CRC-32C (Castagnoli) of other files. For further | |
| details see @doc/format-directory.md@. |
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.
| writePrimVar checksum crc' | ||
|
|
||
| {------------------------------------------------------------------------------- | ||
| Specialised Writers for ChecksumHandle |
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.
Where functions like writeRawPage would previously select the correct handle from the RunBuilder such as forRunKOps runBuilderHandles, the new newRawPage could in theory be passed any ChecksumHandle. It's probably not a problem in practice, as the tests would catch incorrect usage. Still, if you think it's a good idea we could introduce some type safety by adding some newtypes, such as newtype KOpsHandle s h = KOpsHandle (ChecksumHandle s h)
Thinking along those same lines, writeBlob and copyBlob are currently passed a variable holding the offset into the file: this could also be a bogus variable with no connection to the blob file. Maybe for the blob handle, we can introduce data BlobHandle s h = BlobHandle !(PrimVar s Word64) !(ChecksumHandle s h) instead of a newtype around just the ChecksumHandle part
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.
That might be useful. I took them out of the wrapper because otherwise I'd need additional handles for two nonexistent files. Wrapping them up individually would indeed solve that. Would you like me to implement that?
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.
You can implement it, or leave a TODO, what you prefer
|
BTW, the suggestion to change |
eafc509 to
f50d254
Compare
jorisdral
left a comment
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. Let's squash the first 3 commits together, and the last 2 commits together
7664982 to
588171b
Compare

This PR:
ChecksumHandleand its core functions to their own module (Database.LSMTree.Internal.ChecksumHandle).RunBuilder(writeRawPage,writeRawOverflowPages,writeBlob,copyBlob,writeFilter,writeIndexHeader,writeIndexChunk,writeIndexFinal) to only take the arguments they need, rather than the entireRunBuilder.ChecksumHandlemodule. This enables them to be reused by theWriteBufferWriter. (Technically, the latter four are not needed, but it seemed better to generalise and move them for consistency.)