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

Implemented get_blob on sampling rocksdb adapter #732

Merged
merged 2 commits into from
Sep 6, 2024

Conversation

holisticode
Copy link
Contributor

This PR adds a get_blob implementation on the rocksdb Adapter for the sampling service.

Copy link
Member

@bacv bacv left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks

Comment on lines 68 to 77
let blob_bytes = load_blob(
self.settings.blob_storage_directory.clone(),
blob_id.as_ref(),
&column_idx.to_be_bytes(),
)
.await
.unwrap();
Ok(S::deserialize(blob_bytes)
.map(|blob| Some(blob))
.unwrap_or_default())
Copy link
Member

Choose a reason for hiding this comment

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

Just a suggestion for grouping deserialization with the loading. The load_blob().unwrap() can be changed to ?.

Suggested change
let blob_bytes = load_blob(
self.settings.blob_storage_directory.clone(),
blob_id.as_ref(),
&column_idx.to_be_bytes(),
)
.await
.unwrap();
Ok(S::deserialize(blob_bytes)
.map(|blob| Some(blob))
.unwrap_or_default())
let blob = S::deserialize(
load_blob(
self.settings.blob_storage_directory.clone(),
blob_id.as_ref(),
&column_idx.to_be_bytes(),
)
.await?,
)
.map(|b| Some(b))
.unwrap_or_default();
Ok(blob)

Copy link
Member

Choose a reason for hiding this comment

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

Actually your variant is better, only lets change the unwrap and it's good to go!

@holisticode holisticode requested review from bacv and danielSanchezQ and removed request for danielSanchezQ September 6, 2024 18:05
@holisticode holisticode merged commit 49ac81b into master Sep 6, 2024
12 checks passed
@holisticode holisticode deleted the das-rocksdb-getblob branch September 6, 2024 18:31
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.

2 participants