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

Error when checking out invalid snapshot ID #317

Merged
merged 4 commits into from
Oct 24, 2024
Merged

Conversation

dcherian
Copy link
Contributor

@dcherian dcherian commented Oct 23, 2024

Closes #235

ValueError: store error: unsuccessful repository operation: `snapshot not found: `F000BARSPAM123456780``

icechunk/src/repository.rs Outdated Show resolved Hide resolved
@rabernat
Copy link
Contributor

Could we get more colons into the error message please? 😆

@@ -1112,6 +1114,17 @@ async fn all_chunks<'a>(
Ok(existing_array_chunks.chain(new_array_chunks))
}

pub async fn raise_if_invalid_snapshot_id(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpicking: I think Storage is enough here? No need for Send + Sync?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apparently required:

error[E0277]: `dyn icechunk::Storage` cannot be shared between threads safely
    --> icechunk-python/src/lib.rs:364:9
     |
364  | /         pyo3_asyncio_0_21::tokio::future_into_py(py, async move {
365  | |             do_checkout_snapshot(store, snapshot_id).await
366  | |         })
     | |__________^ `dyn icechunk::Storage` cannot be shared between threads safely
     |
     = help: the trait `std::marker::Sync` is not implemented for `dyn icechunk::Storage`, which is required by `{async block@icechunk-python/src/lib.rs:364:54: 364:64}: std::marker::Send`
     = note: required for `&dyn icechunk::Storage` to implement `std::marker::Send`
note: required because it's used within this `async` fn body

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But, I don't get it. It errors in an unrelated function we are not calling?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function does end up calling our new function to make sure snapshot_id is valid during hceckout, is that it?

@dcherian dcherian merged commit c8fcff5 into main Oct 24, 2024
3 checks passed
@dcherian dcherian deleted the invalid-snapshot-id branch October 24, 2024 18:07
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.

store.checkout should raise an error if snapshot_id does not exist
3 participants