Fix truncate tail #33
Conversation
|
There are some tests failing due to the additional validation I added - it looks like we have some work to do to consistently support seal metadata correctly in |
There was a problem hiding this comment.
🚀
Reading and trying to understand all the synchronization mechanisms around the segment operations was so enlightening! 😄
I think one integration test that we need to make at some point is retro compatibility with the on disk format. This could be achieved by saving a few test scenarios with each released version and trying to open/read them in tests in main.
This is a great call. We've not made any changes to disk format since I first released this yet but when we do we should certainly have an upgrade test. I think you are suggesting that it would be worth having that anyway just in case we accidentally change something. Not a bad call. I guess it wouldn't be too hard to write out a test dir into |
| sealed, indexStart, err := ts.Sealed() | ||
| require.NoError(t, err) | ||
| require.True(t, sealed) | ||
| require.NotEqual(t, 0, int(indexStart)) |
There was a problem hiding this comment.
Realised I should also assert that the seg.IndexStart matches the indexStart. It's a bit of a tautology in the sense that segments read from disk would actually populate indexStart from metadata in the first place but in these tests it would actually verify something useful: that the segment we just sealed reported it's index offset consistently and that it was actually updated in the meta data. I'll add that before I merge.
Fixes #31.
The WAL layer assumes that the tail index will be sealed during a tail truncate and marks it as such in the meta store but doesn't actually have a mechanism currently to force the segment writer to append an index block.
This PR fixes that by adding such a mechanism.
Integration Tests
Since this is another issue that escaped due to lack of integration testing between the component layers, I've added a basic initial integration suite that uses WAL with real Segment/Meta/FS in some basic scenarios.
Although basic, two of the scenarios failed without the fix to call
ForceSealin this PR in the same way as #31. There is also at least one scenario that would have failed my initial integration with Consul before I had the snapshot behaviour correct (another integration testing miss).Perhaps in the future we can extend this to cover other scenarios or to use fault injection or fuzzing etc. But for now this is a solid improvement over no integration tests!