Allow multi-read in CollectionView
#4609
Conversation
| let mut updates = self | ||
| .updates | ||
| .try_write() | ||
| .ok_or(ViewError::CannotAcquireCollectionEntry)?; |
There was a problem hiding this comment.
It's great to finally remove this! We can remove the comment "May fail if one subview is already being visited."
There was a problem hiding this comment.
Let's make sure to do another (non-backportable) PR after this one to remove CannotAcquireCollectionEntry.
| } | ||
|
|
||
| Ok(values) | ||
| let values = self.try_load_entries(&keys).await?; |
| match updates.get(short_key) { | ||
| Some(update) => match update { | ||
| Update::Removed => Ok(None), | ||
| _ => Ok(Some(ReadGuardedView::Loaded { |
There was a problem hiding this comment.
using _ in match is an anti-pattern: when you add new variants to an enum compiler won't warn you about mishandling.
There was a problem hiding this comment.
There are more occurences of the same pattern – fix all of them.
There was a problem hiding this comment.
I would generally agree. Here the enum is defined in the same file and unlikely to be changed without checking all the occurrences, but at the same time it is also very easy to replace the few entries.
| .into_iter() | ||
| .zip(keys) | ||
| .map(|(value, key)| match value { | ||
| None => Err(missing_key_error(&key)), |
There was a problem hiding this comment.
I think it should be OK if one of the requested entries is not found, while others are. We shouldn't fail the whole query.
There was a problem hiding this comment.
Imagine if we query for an ERC20-like app with 100 addreses – 99 are present but 1 is missing. Does the whole query fail? It'd be bad.
There was a problem hiding this comment.
That is a change of API.
It should be done in another PR, I think.
Also, right now, we do not have support for GraphQL for Solidity contracts.
There was a problem hiding this comment.
It's not about Solidity, Game of Life does the same thing. Portal could query all users' scores (or batch it) instead of doing 12k separate queries – each of a new user.
There was a problem hiding this comment.
Yes, and?
The Game Of Life is returning an error if 1 of the keys being queried is missing. Changing that would be a change of API.
There was a problem hiding this comment.
It is a breaking change:
- Currently, if one key is missing in the
ReentrantCollectionView, then the whole query fails. - After this PR we have the same behavior.
There was a problem hiding this comment.
The label filters inside MapInput suggests that we return the existing entries at the intersection of all filters. The list of keys being a "filter", it is a bug to return an error if some keys don't exist.
There was a problem hiding this comment.
if one key is missing in the ReentrantCollectionView, then the whole query fails.
Yes but that's a bug (when we use MapInput)
There was a problem hiding this comment.
The structure of the queries is somewhat odd:
- The input to the
fn entriesis anOption<MapInput>. - The
MapInputencapsulates anOption<MapFilters>. - The
MapFiltersencapsulates aVec<K>.
So, you have 3 levels of Option but that does not matter as it gets collapsed for MapView, ReentrantCollectionView, CollectionView as one single Option. All 3 handle the entries in essentially the same way.
It is a little unclear to me why this special structure is needed. There are few calls to the entries function in the code, the examples and related code using Linera. And those calls ask for the whole of entries. In fact all the calls to entries ask for all the keys (so None, wherever that is put in the hierarchy).
There is no notion of filter in GraphQL as a search on the reference https://spec.graphql.org/October2021/ does not show up any matching for filter. So, whatever meaning there is to filter is one that we decide by ourselves.
The SimpleObject macro when applied to a struct, will provide implementations to a BTreeMap, which in case of a missing key will return null. But of course, we are not forced to follow this convention, and if we decide that a filters changes thing, then why not.
There was a problem hiding this comment.
We discussed this internally on slack just one or two days ago.
Motivation
The reading of multiple keys in
CollectionViewdoes not work. This is unfortunate since the underlyingcontainer of
CollectionViewisasync_lock::RwLockwhich allows multiple read or one write. WeCollectionViewshould have the same semantics.
Fixes #4588
Proposal
The following steps are made:
ReadGuardedViewis now an enum. TheLoadedentry is when it is present in updates. TheNotLoadedwhen accessed from storage.NotLoadedcontains some lock so as to avoid a write to be created and lead to incoherent state.try_load_entryof a view on storage, the updates is not changed. But that is fine, since it is anyway in the LRU cache.try_load_entrieshas been introduced since we can now read multiple keys at once.fn entrieshas been simplifed to use those functions.try_load_entriesno longer take values since only references are needed.Clonerequirement have been alleviated inCollectionView/ReentrantCollectionView.Test Plan
The CI.
The unit tests in the code should be sufficient for the functionality.
Release Plan
It addresses an issue that showed up in the GameOfLife. So, moving it to TestNet seems like.a good idea.
Links
None.