Skip to content

Conversation

@andrewbaptist
Copy link

@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
@blathers-crl
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 RaduBerinde and angles-n-daemons and removed request for a team January 30, 2025 20:16
@andrewbaptist andrewbaptist requested review from a team, golgeek, herkolategan, jbowens, michae2, msbutler, mw5h and rharding6373 and removed request for a team, DarrylWong, golgeek, msbutler, mw5h and wenyihu6 January 30, 2025 20:16
@andrewbaptist
Copy link
Author

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

@andrewbaptist
Copy link
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)

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.

Yeah, agreed.. seems like it doesn't belong

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

@rafiss rafiss removed the request for review from a team February 5, 2025 21:36
@rharding6373 rharding6373 removed their request for review January 6, 2026 17:25
@michae2 michae2 removed their request for review January 13, 2026 23:18
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