[Extremely WIP] API Refactor #432
Conversation
| Ok(()) | ||
| } | ||
|
|
||
| pub async fn commit( |
There was a problem hiding this comment.
commit is now consumed, so after committing you need to get a new session.
icechunk/src/store.rs
Outdated
| zarr::{KeyNotFoundError, ListDirItem, StoreError, StoreOptions, StoreResult}, | ||
| }; | ||
|
|
||
| pub struct Store<'a> { |
There was a problem hiding this comment.
This might cause major issues with python bindings, lifetimes arent allowed
icechunk/src/session.rs
Outdated
| } | ||
|
|
||
| impl Session { | ||
| pub fn create_readable_session( |
There was a problem hiding this comment.
| pub fn create_readable_session( | |
| pub fn create_readonly_session( |
I feel like this would be a slightly better name. All sessions are readable, right?
There was a problem hiding this comment.
good catch, yeah thats a relic left over from when i tried to implement these as traits
| let snapshot_id: SnapshotId = match version { | ||
| VersionInfo::SnapshotId(sid) => { | ||
| raise_if_invalid_snapshot_id(self.storage.as_ref(), &sid).await?; | ||
| Ok::<_, RepositoryError>(SnapshotId::from(sid.clone())) |
There was a problem hiding this comment.
so weird that you need to type hint this... are you should Ok(...) doesn't work?
icechunk/src/session.rs
Outdated
|
|
||
| pub fn store(&mut self) -> Store { | ||
| let read_only = self.read_only(); | ||
| let mutexed = Arc::new(RwLock::new(self)); |
There was a problem hiding this comment.
this looks quite suspicious to me, but haven't thought about it enough. This when calling store, you give out the only possible mutable reference to the Session, so you no longer can make changes to the session until the Store is freed. Will have to wrap my head around it, not sure how commit would work for example.
There was a problem hiding this comment.
I really don't like this. I don't know how else to manage though. Need to think through the hierarchy
There was a problem hiding this comment.
OK the only mutable part is the ChangeSet I think, so we can in theory just Arc<Mutex the changeset and then the session is no longer locked. I think
| get_old_chunk(self.storage.as_ref(), node, manifests, coords).await | ||
| } | ||
|
|
||
| pub async fn list_nodes( |
There was a problem hiding this comment.
@paraseba any ideas on how to fix this? This currently is not correct because the iterator grabs the lifetime of the RWLockGuard which is temporary, so returning an iterator does not compile
|
I like the approach here much better now, just have to figure out if there is a way to creature the listing iterators generatively, because otherwise listing is (again) going to cause major lifetime issues |
#422 #221