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

tool: fix up wal dump, add wal dump-merged #3864

Merged
merged 1 commit into from
Aug 26, 2024

Conversation

jbowens
Copy link
Collaborator

@jbowens jbowens commented Aug 19, 2024

Adjust the wal dump tool to error on unrecognized key kinds, and surface errors
encounted more prominently in the output.

Additionally, add a new wal dump-merged command that dumps the merged view of
a WAL that's composed of multiple physical segment files.

Informs #3865.

@jbowens jbowens requested a review from a team as a code owner August 19, 2024 19:16
@jbowens jbowens requested a review from aadityasondhi August 19, 2024 19:16
@cockroach-teamcity
Copy link
Member

This change is Reviewable

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.

:lgtm:

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @aadityasondhi)

@jbowens jbowens force-pushed the wal-dump-invalid-keys branch 2 times, most recently from 0e918c8 to 3b1458a Compare August 20, 2024 17:12
@jbowens jbowens changed the title tool: error on unrecognized key kind tool: fix up wal dump, add wal dump-merged Aug 20, 2024
Copy link
Collaborator Author

@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.

I ended up expanding this while debugging #3865 so that I could reproduce #3865 using the debug tool.

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @aadityasondhi and @RaduBerinde)

@jbowens jbowens force-pushed the wal-dump-invalid-keys branch from 3b1458a to 221f62e Compare August 20, 2024 17:15
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: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @aadityasondhi and @jbowens)


tool/wal.go line 209 at r2 (raw file):

	errf := func(offset wal.Offset, format string, args ...interface{}) {
		prefix := fmt.Sprintf("%s: error: ", offset)
		fmt.Fprintf(stderr, prefix+format+"\n", args...)

This kind of non-constant format string can trip up newer versions of lint checks (see cockroachdb/cockroach#129208).

I'd just pass an error and use errors.Wrap at the callsite and here to add info.


tool/wal.go line 289 at r2 (raw file):

			s, err := rangekey.Decode(ik, value, nil)
			if err != nil {
				errf("%s: error decoding %s", w.fmtKey.fn(ukey), err)

[nit] errf prints to stdout with a newline, unlike the other cases here. Maybe keep the fmt.Fprintf(stdout here and remove it from errf.

If it's not hard, it would be nice to have this case show up in the test.

Adjust the wal dump tool to error on unrecognized key kinds, and surface errors
encounted more prominently in the output.

Additionally, add a new `wal dump-merged` command that dumps the merged view of
a WAL that's composed of multiple physical segment files.
@jbowens jbowens force-pushed the wal-dump-invalid-keys branch from 221f62e to 124c485 Compare August 21, 2024 19:38
Copy link
Collaborator Author

@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.

Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @aadityasondhi and @RaduBerinde)


tool/wal.go line 209 at r2 (raw file):

Previously, RaduBerinde wrote…

This kind of non-constant format string can trip up newer versions of lint checks (see cockroachdb/cockroach#129208).

I'd just pass an error and use errors.Wrap at the callsite and here to add info.

Done


tool/wal.go line 289 at r2 (raw file):

Previously, RaduBerinde wrote…

[nit] errf prints to stdout with a newline, unlike the other cases here. Maybe keep the fmt.Fprintf(stdout here and remove it from errf.

If it's not hard, it would be nice to have this case show up in the test.

Done

I gave the test case a stab but aborted for now because it is tricky

@jbowens jbowens merged commit de41143 into cockroachdb:master Aug 26, 2024
11 checks passed
@jbowens jbowens deleted the wal-dump-invalid-keys branch August 26, 2024 14:06
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.

3 participants