WIP: serialise write buffer when creating a snapshot

[wenkokke]

This is a WIP PR that serialises the write buffer when creating a snapshot.

I'd love some feedback :)

[jorisdral]

Looks promising!

I think this works for writing out the write buffer. It would be good to add a test that exercises writeWriteBuffer directly. Maybe we could test that the WriteBufferWriter and RunBuilder produce the same keyops and blob files?

[jorisdral]

Since the write buffer writer only has two files, I don't think we need the abstractions that we have for runs, like ForRunFiles. To give you an idea of what I mean, I made a small patch that you can download and apply locally using git apply write-buffer-paths.patch, and go from there (if you agree with my assessment)

Gist: https://gist.github.com/jorisdral/cae5737e7b201ed3372d27145d5d7d73

[wenkokke]

Fair enough, I just liked the consistency.

[jorisdral]

I'd pick a smaller name prefix to make these fields less verbose

[wenkokke]

Looks promising!

I think this works for writing out the write buffer. It would be good to add a test that exercises writeWriteBuffer directly. Maybe we could test that the WriteBufferWriter and RunBuilder produce the same keyops and blob files?

Is there any testing infrastructure that would allow me to easily set up the required inputs?

[wenkokke]

Looks promising!

I think this works for writing out the write buffer. It would be good to add a test that exercises writeWriteBuffer directly. Maybe we could test that the WriteBufferWriter and RunBuilder produce the same keyops and blob files?

Do you have any initial feelings regarding the amount of duplication?

[jorisdral]

Looks promising!
I think this works for writing out the write buffer. It would be good to add a test that exercises writeWriteBuffer directly. Maybe we could test that the WriteBufferWriter and RunBuilder produce the same keyops and blob files?

Is there any testing infrastructure that would allow me to easily set up the required inputs?

https://github.com/IntersectMBO/lsm-tree/blob/d11c709cb3c3b91d9f66e94620017e0edd997528/src-extras/Database/LSMTree/Extras/RunData.hs

^ contains some definitions for generating data for runs, which is equivalent to data for write buffers. It's just sorted key/value data without duplicate keys

lsm-tree/test/Test/Database/LSMTree/Internal/Run.hs

Lines 179 to 212 in d11c709

	-- | Loading a run (written out from a write buffer) from disk gives the same
	-- in-memory representation as the original run.
	--
	-- @openFromDisk . flush === flush@
	prop_WriteAndOpen ::
	FS.HasFS IO h
	-> FS.HasBlockIO IO h
	-> RunData KeyForIndexCompact SerialisedValue SerialisedBlob
	-> IO Property
	prop_WriteAndOpen fs hbio wb =
	withRun fs hbio (simplePath 1337) (serialiseRunData wb) $ \written ->
	withTempRegistry $ \reg -> do
	let paths = Run.runRunFsPaths written
	paths' = paths { runNumber = RunNumber 17}
	hardLinkRunFiles reg fs hbio paths paths'
	loaded <- openFromDisk fs hbio CacheRunData (simplePath 17)
	(RefCount 1 @=?) =<< readRefCount (runRefCounter written)
	(RefCount 1 @=?) =<< readRefCount (runRefCounter loaded)
	runNumEntries written @=? runNumEntries loaded
	runFilter written @=? runFilter loaded
	runIndex written @=? runIndex loaded
	writtenKOps <- readKOps Nothing written
	loadedKOps <- readKOps Nothing loaded
	assertEqual "k/ops" writtenKOps loadedKOps
	-- make sure runs get closed again
	removeReference loaded
	-- TODO: return a proper Property instead of using assertEqual etc.
	return (property True)

^ this test could give you an idea of how to use the run data.

[jorisdral]

Looks promising!
I think this works for writing out the write buffer. It would be good to add a test that exercises writeWriteBuffer directly. Maybe we could test that the WriteBufferWriter and RunBuilder produce the same keyops and blob files?

Do you have any initial feelings regarding the amount of duplication?

Oops, yes, I meant to write about that but I forgot. I think it's okay to have the duplication for now, as long as we

• Put down a TODO (maybe in the module header?) to investigate whether we can reduce the amount of duplication. I might have some ideas, but it would mean a larger refactoring effort which I don't think is worth it to do right away
• Establish some correspondence between RunBuilder and WriteBufferWriter, hence the suggestion to add a test

[wenkokke]

Looks promising!

I think this works for writing out the write buffer. It would be good to add a test that exercises writeWriteBuffer directly. Maybe we could test that the WriteBufferWriter and RunBuilder produce the same keyops and blob files?

Do you have any initial feelings regarding the amount of duplication?

Oops, yes, I meant to write about that but I forgot. I think it's okay to have the duplication for now, as long as we

• Put down a TODO (maybe in the module header?) to investigate whether we can reduce the amount of duplication. I might have some ideas, but it would mean a larger refactoring effort which I don't think is worth it to do right away

• Establish some correspondence between RunBuilder and WriteBufferWriter, hence the suggestion to add a test

Fair! I still think the generalisation of the run infrastructure to permit empty filters and indices might be worthwhile, especially with SPJ's recent "specialise with value" extension.

[jorisdral]

Looks promising!

I think this works for writing out the write buffer. It would be good to add a test that exercises writeWriteBuffer directly. Maybe we could test that the WriteBufferWriter and RunBuilder produce the same keyops and blob files?

Do you have any initial feelings regarding the amount of duplication?

Oops, yes, I meant to write about that but I forgot. I think it's okay to have the duplication for now, as long as we

• Put down a TODO (maybe in the module header?) to investigate whether we can reduce the amount of duplication. I might have some ideas, but it would mean a larger refactoring effort which I don't think is worth it to do right away
• Establish some correspondence between RunBuilder and WriteBufferWriter, hence the suggestion to add a test

Fair! I still think the generalisation of the run infrastructure to permit empty filters and indices might be worthwhile, especially with SPJ's recent "specialise with value" extension.

True, though we wouldn't be able to take advantage of that on older GHCs... unless that stuff is backported

[wenkokke]

Looks promising!

I think this works for writing out the write buffer. It would be good to add a test that exercises writeWriteBuffer directly. Maybe we could test that the WriteBufferWriter and RunBuilder produce the same keyops and blob files?

Do you have any initial feelings regarding the amount of duplication?

Oops, yes, I meant to write about that but I forgot. I think it's okay to have the duplication for now, as long as we

• Put down a TODO (maybe in the module header?) to investigate whether we can reduce the amount of duplication. I might have some ideas, but it would mean a larger refactoring effort which I don't think is worth it to do right away

• Establish some correspondence between RunBuilder and WriteBufferWriter, hence the suggestion to add a test

Fair! I still think the generalisation of the run infrastructure to permit empty filters and indices might be worthwhile, especially with SPJ's recent "specialise with value" extension.

True, though we wouldn't be able to take advantage of that on older GHCs... unless that stuff is backported

We could do something a bit unhinged and encode the flag using instances of a type class. That way it's easily backwards compatible.

[wenkokke]

Added the test prop_WriteRunEqWriteWriteBuffer that compares serialisation as Run versus serialisation as WriteBuffer, which fails:

lsm-tree
  Database.LSMTree.Internal.Run
    Write buffer to disk
      prop_WriteRunEqWriteWriteBuffer: FAIL (0.01s)
        *** Failed! (after 4 tests and 2 shrinks):
        Exception:
          Assertion failed
          CallStack (from HasCallStack):
            assert, called at src/Database/LSMTree/Internal/RawBytes.hs:157:5 in lsm-tree-0.1.0.0-inplace:Database.LSMTree.Internal.RawBytes
        WB {unWB = fromList [(SerialisedKey [],Delete)]}
        Use --quickcheck-replay="(SMGen 4977141403849015630 6713696729127803493,3)" to reproduce.

[jorisdral]

Added the test prop_WriteRunEqWriteWriteBuffer that compares serialisation as Run versus serialisation as WriteBuffer, which fails:

The assertion at the specified line of the code asserts that keys should be longer than 8 bytes. The Arbitrary instance for WriteBuffer probably does not satisfy this requirement... Your best bet is probably to create a WriteBuffer from RunData. I can help with that if needed