Repository navigation
wal: reuse the anchor-only tail instead of deleting it - #739
Conversation
HagarMeir
left a comment
HagarMeir
left a comment
There was a problem hiding this comment.
Thanks for splitting this out. The fix works, but the two mechanisms overlap: once the WAL has more than one file, Open deletes the anchor-only last file, so ReadAll never reuses it and creates it again on every restart. I suggest keeping only one of the two. Details and a few smaller points are inline.
Written by Claude (an AI assistant) on behalf of @HagarMeir.
| // anchor-only file leaves the CRC chain of the files before it untouched. | ||
| // | ||
| // The last file of the WAL is kept even when it is an anchor-only one: an empty WAL still has to be | ||
| // a WAL of at least one file, and ReadAll() reuses that file instead of creating a fresh one. |
There was a problem hiding this comment.
This only holds while the WAL is a single file. With [1 (records), 2 (anchor only)], removeAnchorOnlyTailFiles deletes 2, then ReadAll sees that 1 has records and creates 2 again (create + fsync). The count stays flat, but that's because of delete-and-recreate, not reuse. Maybe have Open remove only the anchor-only files before the last one, or drop the Open cleanup and rely on the reuse branch alone?
| return nil, err | ||
| } | ||
|
|
||
| if err = wal.removeAnchorOnlyTailFiles(dirPath); err != nil { |
There was a problem hiding this comment.
Open used to leave the directory untouched; now it can delete files before any write. The test harness calls Setup() again without closing the previous WAL, so a second Open can unlink the file the old instance is still writing to. Not a production issue as far as I can see, but worth a note in the Open doc.
| return err | ||
| } | ||
|
|
||
| if lastFileInfo.Size() == crcAnchorRecordSize() { |
There was a problem hiding this comment.
Here anchor-only means Size() == crcAnchorRecordSize(), while Open uses isAnchorOnlyFile (parse, then expect EOF). Could we use one check for both, so they can't drift apart?
| "the WAL must still carry the same proposal in flight") | ||
| // The first restart after a proposal moves the write head to a new file. From | ||
| // then on ReadAll reuses that anchor-only file, so further restarts add none. | ||
| require.LessOrEqual(t, walLogFileCount(t, walDir), filesBeforeRestart+1, |
There was a problem hiding this comment.
With a single restart, <= filesBeforeRestart+1 looks like it would pass on main too, since main adds exactly one file per restart. Several restarts with a flat count, or <= filesBeforeRestart, would catch a regression. (Also, the comment above says ReadAll reuses the file, but per the note on writeaheadlog.go it gets recreated.)
| } | ||
|
|
||
| // TestRestartingNodeThatDoesNotProposeDoesNotGrowWAL covers R07.1 and R15i on | ||
| // real nodes: a node in a network partition neither proposes nor syncs, so its |
There was a problem hiding this comment.
Nits: Restart() syncs from the shared in-memory ledger whatever the loss probability is, so the node does sync. R07.1/R15i (here and at line 154) aren't defined anywhere in the repo, so they should be dropped or explained.
5a910fa to
63c000c
Compare
Open no longer removes anchor-only tail files. Deleting the last one made ReadAll create a new file on every restart, so the cleanup and the reuse branch overlapped. Keep the reuse branch as the only mechanism and drop the now-unused isAnchorOnlyFile helper. Strengthen the in-flight proposal restart test to do ten restarts with a flat file count, correct the partition description, and remove the undefined requirement IDs from the test comments. Signed-off-by: Fedor Partanskiy <fredprtnsk@gmail.com>
63c000c to
f6a4576
Compare
pfi79
changed the title
comments have been fix and the description has been changed |
|
Thanks, this addresses my comments. A few small things you could fix before merge: the commit message and the description describe the change from the previous revision ("instead of deleting it", "Open no longer removes…") rather than from main, and the same goes for the test comment "Open no longer deletes files". "The audit measurement" and "the regression from the brief" in the test comments could be reworded too. Written by Claude (an AI assistant) on behalf of @HagarMeir. |
Open no longer removes anchor-only tail files. Deleting the last one made
ReadAll create a new file on every restart, so the cleanup and the reuse branch
overlapped. Keep the reuse branch as the only mechanism and drop the now-unused
isAnchorOnlyFile helper.
Strengthen the in-flight proposal restart test to do ten restarts with a flat
file count, correct the partition description, and remove the undefined
requirement IDs from the test comments.
#736