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

storage: move ExternalIODir into StorageConfig #139980

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

Conversation

andrewbaptist
Copy link
Collaborator

@andrewbaptist andrewbaptist commented Jan 28, 2025

Previously the ExternalIODir was stored as a variable inside the
BaseConfig struct and updated based on the StoreSpecs if it wasn't
explicitly set. This commit moves it into the StorageConfig and removes
it from the BaseConfig.

Epic: CRDB-41111

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@andrewbaptist andrewbaptist force-pushed the 2025-01-28-external-io branch 5 times, most recently from dc2fc79 to 4d2b81a Compare January 29, 2025 14:51
Copy link

blathers-crl bot commented Jan 29, 2025

Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@andrewbaptist andrewbaptist force-pushed the 2025-01-28-external-io branch 7 times, most recently from 9435ee1 to b831e3c Compare January 30, 2025 19:42
@andrewbaptist andrewbaptist changed the title 2025 01 28 external io storage: move ExternalIODir into StorageConfig Jan 30, 2025
@andrewbaptist andrewbaptist marked this pull request as ready for review January 30, 2025 20:16
@andrewbaptist andrewbaptist requested review from a team as code owners January 30, 2025 20:16
@andrewbaptist andrewbaptist requested review from angles-n-daemons and RaduBerinde and removed request for a team January 30, 2025 20:16
@andrewbaptist
Copy link
Collaborator Author

@RaduBerinde FYI - I'm investigating the failure in TestChangefeedExecLocality related to the change in ServerArgsPerNode.

@andrewbaptist
Copy link
Collaborator Author

The failure in TestChangefeedExecLocality appears to be a random flake under stress+race and is unrelated to my change (other than by touching the file it caused CI to stress+race this file). Since it was a timeout rather than anything actionable, I'm going to ignore it for this PR's perspective.

Previously the ExternalIODir was stored as a variable inside the
BaseConfig struct and updated based on the StoreSpecs if it wasn't
explicitly set. This commit moves it into the StorageConfig and removes
it from the BaseConfig.

Epic: CRDB-41111

Release note: None
@andrewbaptist andrewbaptist force-pushed the 2025-01-28-external-io branch from b831e3c to 523550f Compare January 31, 2025 21:32
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.

I am not sure that ExternalIODir belongs in storage.. It is not directly related to the stores. @jbowens WDYT? This is the definition:

	// ExternalIODir is the local file path under which remotely-initiated
	// operations that can specify node-local I/O paths (such as BACKUP, RESTORE
	// or IMPORT) can access files.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @angles-n-daemons, @golgeek, @herkolategan, @jbowens, @michae2, @msbutler, and @rharding6373)

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