refactor: use exact base-scoped store bindings#6422
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
westonpace
left a comment
There was a problem hiding this comment.
This seems to be ok as is but I have a few questions.
Is it possible to set an option for the default base? Or would you need to override that option in every base?
Can you mask an option to hide it from the bases? For example, maybe the default storage options has a proxy_url but you don't want to use that in the non-default bases?
|
Thank you @westonpace for the review, I polished the design a bit. Can you talk another look? |
westonpace
left a comment
There was a problem hiding this comment.
I like the simpler and more flexible approach (ability to set other object store params). I still don't see any way to "unset" a storage option using an override but this is not a blocking concern.
| /// Set runtime-only object store params for a specific registered base path. | ||
| /// | ||
| /// These params are not persisted in the manifest. They are used whenever | ||
| /// the dataset resolves an object store for the given `BasePath.path`. |
There was a problem hiding this comment.
Can we describe how the two sets of options are merged better? We have two ObjectStoreParams instances now, the dataset store params and the override params. What actually gets used?
If I'm reading the code correctly I think:
- All options that are not storage_options (e.g. block_size, use_constant_size_upload_parts) are replaced entirely by the override.
- Storage options are handled differently. The value is the union of the two sets of params. If a storage_option is set in both then the value in the override is preferred.
There was a problem hiding this comment.
You're right that the old wording was ambiguous. In the current implementation, per-base bindings are exact ObjectStoreParams keyed by BasePath.path, and dataset-level params are only the fallback. There is no merge between dataset-level and per-base params. I updated the comments in the PR to make that explicit.
This changes per-base runtime configuration to use exact
ObjectStoreParamsbindings keyed byBasePath.pathinstead of per-base storage option overrides. Dataset-level and write-level store params now act only as fallbacks, while reads, target-base writes, and external blob resolution all consult the same base-scoped binding model.This keeps provider-specific runtime state out of the manifest and follows the direction in discussion #6307 to keep
BasePathfocused on identity.