Skip to content
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

record: add WAL read ahead usage, new chunk format, and major version #4336

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

EdwardX29
Copy link
Contributor

Add usages of WAL readAheadForCorruption to record.Next() and record.Read(). Write a new chunk format to include sync offsets for reading ahead. Add a new format major version for the new chunk format.

Followup to: #4331

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@EdwardX29 EdwardX29 force-pushed the edwardx29/wal-sync-usage branch 9 times, most recently from 86ad47a to 08381cb Compare February 13, 2025 02:44
@EdwardX29 EdwardX29 marked this pull request as ready for review February 13, 2025 15:58
@EdwardX29 EdwardX29 requested a review from a team as a code owner February 13, 2025 15:58
@EdwardX29 EdwardX29 changed the title add WAL read ahead usage, new chunk format, and major version record: add WAL read ahead usage, new chunk format, and major version Feb 13, 2025
Copy link
Collaborator

@jbowens jbowens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 11 of 18 files at r1.
Reviewable status: 11 of 18 files reviewed, 8 unresolved discussions (waiting on @aadityasondhi)


format_major_version.go line 205 at r1 (raw file):

	// chunk wire format that encodes an additional "Synced Offset" field which acts
	// as a commitment that the WAL should have been synced up until the offset.
	FormatWalSyncChunks

nit: let's capitalize this as FormatWALSyncChunks (it's idiomatic in Go to capitalize initialisms like WAL)


open.go line 369 at r1 (raw file):

		Logger:               opts.Logger,
		EventListener:        walEventListenerAdaptor{l: opts.EventListener},
		WritingWalSyncChunks: FormatMajorVersion(d.mu.formatVers.vers.Load()) >= FormatWalSyncChunks,

nit: ditto here, let's make this field name WriteWALSyncChunks or the like


open.go line 937 at r1 (raw file):

			// errors from EOF in order to recognize that the record was
			// truncated and to avoid replaying subsequent WALs, but want
			// to otherwise treat them like EOF.

let's expand and edit this existing comment. Maybe something like:

			// It is common to encounter a zeroed or invalid chunk due to WAL
			// preallocation and WAL recycling. However zeroed or invalid chunks
			// can also be a consequence of corruption / disk rot. When the log
			// reader encounters one of these cases, it attempts to disambiguate
			// by reading ahead looking for a future record. If a future chunk
			// indicates the chunk at the original offset should've been valid, it
			// surfaces record.ErrInvalidChunk or record.ErrZeroedChunk. These
			// errors are always indicative of corruption and data loss.
			//
			// Otherwise, the reader surfaces io.ErrUnexpectedEOF indicating that
			// the WAL terminated uncleanly and ambiguously. If the WAL is the
			// most recent logical WAL, the caller passes in (strictWALTail=false),
			// indicating we should tolerate the unclean ending. If the WAL is an
			// older WAL, the caller passes in (strictWALTail=true), indicating that
			// the WAL should have been closed cleanly, and we should interpret
			// the `io.ErrUnexpectedEOF` as corruption and stop recovery.

record/log_writer.go line 478 at r1 (raw file):

	// syncedOffset is the offset in the log that is durably synced after a
	// flush. This member is used to write the Wal Sync chunk format's "Offset"

nit: "WAL"


record/log_writer.go line 1039 at r1 (raw file):

}

func (w *LogWriter) emitFragmentWalSync(n int, p []byte) (remainingP []byte) {

nit: emitFragmentSyncOffsets

do we have any LogWriter unit tests that exercise this? looks like the code coverage profile indicates this function doesn't have coverage


record/testdata/walSync line 463 at r1 (raw file):


nit: let's remove extra whitespace between datadriven commands


wal/wal.go line 152 at r1 (raw file):

	// WritingWalSyncChunks represents whether to write the WAL sync chunk format.
	// It is plumbed down from wal.Options to record.newLogWriter.
	WritingWalSyncChunks bool

nit: WriteWALSyncOffsets


record/record.go line 288 at r1 (raw file):

				if r.end+recyclableHeaderSize > r.n {
					// Skip the rest of the block if the recyclable header size does not
					// fit within it. The end of a log will be zeroed out if the log writer

nit: "The end of a block..."

@EdwardX29 EdwardX29 force-pushed the edwardx29/wal-sync-usage branch 2 times, most recently from cc79a2e to caa0df8 Compare February 14, 2025 18:39
@EdwardX29 EdwardX29 force-pushed the edwardx29/wal-sync-usage branch from caa0df8 to 8b518bb Compare February 18, 2025 15:37
Add usages of WAL readAheadForCorruption to record.Next() and
record.Read(). Write a new chunk format to include sync offsets
for reading ahead. Add a new format major version for the new
chunk format.
@EdwardX29 EdwardX29 force-pushed the edwardx29/wal-sync-usage branch from 8b518bb to 4fa7ac2 Compare February 18, 2025 16:23
Copy link
Member

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 7 of 19 files reviewed, 10 unresolved discussions (waiting on @aadityasondhi, @EdwardX29, and @jbowens)


a discussion (no related file):
:lgtm:


record/log_writer.go line 54 at r2 (raw file):

const (

[nit] these could both be var noOpSequence = []byte{255,255,255,255}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants