Fix blob storage #3502
Fix blob storage #3502
Conversation
There was a problem hiding this comment.
Looks good to me!
The assumption that after clone_with_root_key we have exclusive access is not something that storage guarantees, is it? It's just that in our use case, all cloned stores are for root views, and those are always only accessed by a single worker?
| /// Whether we have exclusive R/W access to the keys under the root key of the store. | ||
| has_exclusive_access: bool, |
There was a problem hiding this comment.
The use of the terminology has_exclusive_access is adequate for the terminology.
But the use in the case !use_exclusive_access makes sense only in a system where data is written in keys are only written, never modified, never erased. That needs to be said. But yes, it is better than cache_key_absence or PR #3496.
The problem of this design is that it forces an API. Conceivably, we could do a clone_with_root_key and not have exclusive access to it. Also, we could access to a store and have access to it. Therefore, I think it would make sense to have that in the input parameter to the LruCachingConfig.
There was a problem hiding this comment.
We need to rename clone_with_root_key. In fact the types returned by connect and clone_with_root_key should probably be different.
| if !self.has_exclusive_access { | ||
| return Err(JournalConsistencyError::JournalRequiresExclusiveAccess.into()); | ||
| } |
There was a problem hiding this comment.
The action is indeed necessary for the system to work correctly.
However, I would say that this kills the interest of journaling.
The problem is that what is stored as blobs is large data like Wasm compiled code. Given the limits on size for the fastpath this could be a problem. Still, I have not been able to enable the problem.
Right now, I do not see how to address this issue and have journal and non-exclusive access.
Co-authored-by: Andreas Fackler <[email protected]> Signed-off-by: Mathieu Baudet <[email protected]>
Co-authored-by: Andreas Fackler <[email protected]> Signed-off-by: Mathieu Baudet <[email protected]>
| let store = linera_views::dynamo_db::DynamoDbStore::new_test_store() | ||
| .await | ||
| .unwrap(); | ||
| let store = store.clone_with_root_key(&[]).unwrap(); |
There was a problem hiding this comment.
I do not think that is needed as the run_big_write_read is very much not cached.
There was a problem hiding this comment.
This seems to be required:
https://github.com/linera-io/linera-protocol/actions/runs/13734903398/job/38417381624
There was a problem hiding this comment.
It's more about unsafe journaling actually
linera-views/tests/store_tests.rs
Outdated
| use linera_views::store::AdminKeyValueStore as _; | ||
|
|
||
| for scenario in get_random_test_scenarios() { | ||
| let store = linera_views::scylla_db::ScyllaDbStore::new_test_store() | ||
| .await | ||
| .unwrap(); | ||
| let store = store.clone_with_root_key(&[]).unwrap(); | ||
| run_reads(store, scenario).await; | ||
| } | ||
| } | ||
|
|
||
| #[cfg(with_scylladb)] | ||
| #[tokio::test] | ||
| async fn test_reads_scylla_db_no_journaling() { |
There was a problem hiding this comment.
Running locally those tests, it is impossible to see some differences in the timing. And running both tests takes the same runtime as one single test.
Also, I object to the no_journaling terminology:
- The journaling is still present in the journaling. If we are in exclusive access then operation fail.
- The difference is potentially about caching not journaling. And there is still caching, just caching of values, not absence of values.
- But actually there is no difference. The difference between exclusive access and no exclusive access is about the checking of absence, and that caching of absence is not going to be present.
There was a problem hiding this comment.
ok fair. let me rename to test_reads_scylla_db_no_root_key
| match &self.lru_read_values { | ||
| None => (), | ||
| Some(lru_read_values) => { | ||
| let mut lru_read_values = lru_read_values.lock().unwrap(); | ||
| lru_read_values.has_exclusive_access = true; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This can be shortened as
if let Some(lru_read_values) = &self.lru_read_values {
let mut lru_read_values = lru_read_values.lock().unwrap();
lru_read_values.has_exclusive_access = true;
}This could also be inlined in the clone_with_root_key.
Yes that's correct |
Motivation
cache_key_absenceto the LruCachingConfig. #3496, LRU-caching is incorrect outside views.Proposal
On top of #3501, we simply deactivate the features that require exclusive access to an object in storage.
Nonebut instead forget about the deleted key. (Not that we delete blobs but who knows in the future.)We may want to rename
connectandclone_with_root_keyslater in another PR.Test Plan
Release Plan
In principle, this chould be backported to the latest
testnetbranch.