feat(storage): implement opendal resolving storage#2231
feat(storage): implement opendal resolving storage#2231blackmwk merged 9 commits intoapache:mainfrom
Conversation
| props: HashMap<String, String>, | ||
| /// Cache of scheme → storage mappings. | ||
| #[serde(skip, default)] | ||
| storages: RwLock<HashMap<String, Arc<OpenDalStorage>>>, |
There was a problem hiding this comment.
This should be multi map? For example, we may need to support both s3 and s3a for S3 storage.
There was a problem hiding this comment.
The key here is a string representing a scheme, you can have both within a map:
("s3", OpenDalS3Storage),
("s3a", AnotherOpenDalS3Storage)
Or we are thinking of mapping one scheme to multiple storages?
There was a problem hiding this comment.
I was thinking we should map them into same storage? A storage instance has a lot of resources inside, like connection pool, etc.
There was a problem hiding this comment.
Currently there is a configured_scheme for OpenDalStorage::{S3 , Azdls}, the path it handles should match the configured scheme, so technically it shouldn't be using the same storage instance if the schemes are different.
https://github.com/apache/iceberg-rust/blob/main/crates/storage/opendal/src/lib.rs#L110
I'm not quite sure about the reason tho, maybe it's an OpenDal limitation? I think we can improve this in a different PR if needed
There was a problem hiding this comment.
IIRC the configured_scheme was a legacy setting from before we refactor Storage trait. I think we no longer need this field since the Stroage now accepts the full url. Please create an issue to track it.
| props: HashMap<String, String>, | ||
| /// Cache of scheme → storage mappings. | ||
| #[serde(skip, default)] | ||
| storages: RwLock<HashMap<String, Arc<OpenDalStorage>>>, |
There was a problem hiding this comment.
I was thinking we should map them into same storage? A storage instance has a lot of resources inside, like connection pool, etc.
2 participants
Which issue does this PR close?
What changes are included in this PR?
Are these changes tested?
Added a new test