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: adapt find command to explicitly track paths, filenames #3315

Merged
merged 1 commit into from
Feb 20, 2024

Conversation

jbowens
Copy link
Collaborator

@jbowens jbowens commented Feb 19, 2024

Previously, the find tool used a map[base.DiskFileNum]string to record file paths across all file types. With #3230, WALs will need separate file path tracking because a single file number may actually consist of several physical files at several unique file paths. To avoid any confusion after this change is made, paths are now propagated alongside disk file numbers.

Previously, the find tool used a map[base.DiskFileNum]string to record file
paths across all file types. With cockroachdb#3230, WALs will need separate file path
tracking because a single file number may actually consist of several physical
files at several unique file paths. To avoid any confusion after this change is
made, paths are now propagated alongside disk file numbers.
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@jbowens jbowens requested review from a team and aadityasondhi February 19, 2024 21:07
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:

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

Copy link
Collaborator

@sumeerbhola sumeerbhola 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: all files reviewed, 1 unresolved discussion (waiting on @aadityasondhi and @jbowens)


tool/find.go line 80 at r1 (raw file):

type fileLoc struct {
	base.DiskFileNum
	path string

won't we need the FS too? Is that for a future PR when we add a second FS to pebble.Options, and this tool lists logs in both?

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.

TFTRs!

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @aadityasondhi and @sumeerbhola)


tool/find.go line 80 at r1 (raw file):

Previously, sumeerbhola wrote…

won't we need the FS too? Is that for a future PR when we add a second FS to pebble.Options, and this tool lists logs in both?

yeah, I'm planning on either adding it in that PR or further tweaking the wal.Logs type so that the find tool doesn't use fileLoc for WALs at all. I'm not sure what'll shake out yet, but I figured regardless it would be confusing to maintain the base.DiskFileNum -> path map if it didn't hold for all disk file nums.

@jbowens jbowens merged commit 8cd9719 into cockroachdb:master Feb 20, 2024
11 checks passed
@jbowens jbowens deleted the find-diskfilenum branch February 20, 2024 15:26
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