fix: move confMergeSchedule next to confMergePolicy #708
Conversation
wenkokke
requested review from
dcoutts,
jorisdral,
mheinzel and
recursion-ninja
as code owners
| (cfs :: MergeSchedule)) | ||
| = TableConfig cmp csz cwba cbfa cfit dcp cfs | ||
| override confDiskCachePolicy' TableConfig {..} | ||
| = TableConfig {confDiskCachePolicy = confDiskCachePolicy', ..} |
There was a problem hiding this comment.
This comes with a risk of forgetting to override all relevant places when new fields are added. Not super likely that the policy occurs more than once in the config, but maybe still a good habit to a be more explicit about these things. For previous discussion and an alternative approach that is not quite as verbose: #641 (comment))
| confMergePolicy <- decodeVersioned v | ||
| confSizeRatio <- decodeVersioned v | ||
| confWriteBufferAlloc <- decodeVersioned v | ||
| confBloomFilterAlloc <- decodeVersioned v | ||
| confFencePointerIndex <- decodeVersioned v | ||
| confDiskCachePolicy <- decodeVersioned v | ||
| confMergeSchedule <- decodeVersioned v | ||
| pure TableConfig {..} |
There was a problem hiding this comment.
I think it would be okay to make a change to the serialisation format now, before the library gets released.
But I kind of like that you make the order explicit here, instead of it implicitly being defined in the datatype declaration (which had a risk of changing it without being aware of the consequences).
| encode TableConfig {..} = | ||
| encodeListLen 7 | ||
| <> encode mergePolicy | ||
| <> encode sizeRatio | ||
| <> encode writeBufferAlloc | ||
| <> encode bloomFilterAlloc | ||
| <> encode fencePointerIndex | ||
| <> encode diskCachePolicy | ||
| <> encode mergeSchedule | ||
| where | ||
| TableConfig | ||
| mergePolicy | ||
| sizeRatio | ||
| writeBufferAlloc | ||
| bloomFilterAlloc | ||
| fencePointerIndex | ||
| diskCachePolicy | ||
| mergeSchedule | ||
| = config | ||
| <> encode confMergePolicy | ||
| <> encode confSizeRatio | ||
| <> encode confWriteBufferAlloc | ||
| <> encode confBloomFilterAlloc | ||
| <> encode confFencePointerIndex | ||
| <> encode confDiskCachePolicy | ||
| <> encode confMergeSchedule |
There was a problem hiding this comment.
The risk here is that one might forget to encode a new field. However, the decodeVersioned implementation would fail to compile, hopefully making it clear that encode also needs to be updated.
There was a problem hiding this comment.
The point here is that if you forget to encode a new field, the decode test fails, which hopefully lets you know that you messed up the encode function. Moreover, these functions are much less brittle depending on the order in the datatype. I've elaborated the pattern match to be explicit about names and total, which is much more verbose, but combines the best of both.
wenkokke
force-pushed
the
wenkokke/move-conf-merge-schedule
branch
4 times, most recently
from
ee3fdf3 to
0c41a34
Compare
mheinzel
left a comment
mheinzel
left a comment
There was a problem hiding this comment.
One comment, but this seems good to go either way.
| confWriteBufferAlloc = confWriteBufferAlloc, | ||
| confBloomFilterAlloc = confBloomFilterAlloc, | ||
| confFencePointerIndex = confFencePointerIndex, | ||
| confDiskCachePolicy = confDiskCachePolicy' |
There was a problem hiding this comment.
The changed and unchanged lines seem super similar, which makes it a bit hard to see where the update happens. Two ideas for making the updated field stands out more:
- use a different name for the policy argument
- use
NamedFieldPunsin the other lines, so they're justconfMergePolicy,etc., making it super clear that the field is untouched
There was a problem hiding this comment.
Used the NamedFieldPuns suggestion.
wenkokke
force-pushed
the
wenkokke/move-conf-merge-schedule
branch
3 times, most recently
from
375cb74 to
0ce2354
Compare
wenkokke
force-pushed
the
wenkokke/move-conf-merge-schedule
branch
from
0ce2354 to
75d28a7
Compare
This PR reorders the fields in
TableConfigto ensure that theMergePolicyandMergeScheduleare adjacent.