`linera-views`: add local variants of store traits #1843

Twey · 2024-03-28T19:43:16Z

Motivation

In order to support non-Send Linera view stores on the Web, we need local variants of the store traits that don't require Send.

Proposal

Replace async-trait with native AFIT, and use trait-variant to automatically create local and non-local variants.

Test Plan

CI build should catch regressions.

Release Plan

Need to bump the major/minor version number in the next release of the crates.

This changes the trait interfaces, which is a backward-incompatible change (for people using linera-views directly).

Links

reviewer checklist

Twey · 2024-04-01T15:06:51Z

linera-views/src/common.rs

+    fn read_value<V: DeserializeOwned>(
+        &self,
+        key: &[u8],
+    ) -> impl Future<Output = Result<Option<V>, E>>
    where
+        Self: Sync,
        E: From<bcs::Error>,


We use fn() -> impl Future instead of async fn() here because default implementations of async fn confuse trait_variant, but it seems fine with default implementations of -> impl Trait fns.

Would it make sense to add a comment in the code?

I think it should be okay since if someone tries to change this it will fail to compile.

(and this applies to several places, so I'd have to copy-paste the comment several times in order to make sure the reader caught it)

I think Mathieu Baudet meant adding a TODO(#XXX) that corresponds to considering the resolution of that bug. There are several pending issues here like the confusion of the trait and having a mention could help in dealing with that. The idea is that if something needs you 10 minutes to figure out then a comment is worthwhile.

I didn't add an issue because we're currently blocked on the upstream bug so there's not really much for us TODO — but I've added a comment above the default implementations with a link to the bug to save future readers the time of figuring it out :)

MathieuDutSik

I guess that you need have some code working with non Send stuff. So, you have your reasons. But the works also applies to the Sync trait and it would be nice to mention that.
Also, the boxed() correction is not part of the description of the PR.

MathieuDutSik · 2024-04-01T15:38:10Z

linera-views/src/scylla_db.rs

+        join_all(handles)
+            .await
+            .into_iter()
+            .collect::<Result<_, _>>()


This (and other small rewrites in this PR) were mandated by Clippy after removing async-trait!

MathieuDutSik · 2024-04-01T15:40:38Z

linera-views/src/dynamo_db.rs

@@ -24,7 +24,7 @@ use aws_sdk_dynamodb::{
    Client,
 };
 use aws_smithy_types::error::operation::BuildError;
-use futures::future::join_all;
+use futures::future::{join_all, FutureExt as _};


Why do we need the FutureExt as _.

In order to use FutureExt::boxed.

MathieuDutSik · 2024-04-01T15:49:20Z

linera-views/src/common.rs

-    async fn delete_all(config: &Self::Config) -> Result<(), Self::Error> {
-        for namespace in Self::list_all(config).await? {
-            Self::delete(config, &namespace).await?;
+    fn delete_all(config: &Self::Config) -> impl Future<Output = Result<(), Self::Error>> {


Is that really absolutely needed?
The async fn syntax is familiar and working well.

See the comment here — it's because trait-variant gets confused with default implementations of async fns.

(I would also prefer to keep it as async fn if we could! I think we should undo this change as soon as trait-variant supports default-implemented async fns.)

Pertinent trait-variant issue: rust-lang/impl-trait-utils#17

MathieuDutSik · 2024-04-01T15:52:03Z

linera-views/src/dynamo_db.rs

@@ -539,6 +538,7 @@ impl DynamoDbStoreInternal {
            .expression_attribute_values(":prefix", AttributeValue::B(Blob::new(key_prefix)))
            .set_exclusive_start_key(start_key_map)
            .send()
+            .boxed()


Is the boxed work independent on the work on LocalKeyValueStore?

You're right, I've moved it to #1860 :)

Twey · 2024-04-02T00:07:40Z

the works also applies to the Sync trait and it would be nice to mention that.

I think the original async-trait-based implementation of these traits already assumed that Self: Sync — certainly the implementation needs Self to be Sync in order for the future to be Send. As far as I know, the only change is that the Sync bound is now a constraint on the default function implementation rather than a requirement for the trait itself.

MathieuDutSik

Thank you for this interesting work.

MathieuDutSik · 2024-04-02T13:46:55Z

linera-views/src/common.rs

+    fn read_value<V: DeserializeOwned>(
+        &self,
+        key: &[u8],
+    ) -> impl Future<Output = Result<Option<V>, E>>
    where
+        Self: Sync,
        E: From<bcs::Error>,


I think Mathieu Baudet meant adding a TODO(#XXX) that corresponds to considering the resolution of that bug. There are several pending issues here like the confusion of the trait and having a mention could help in dealing with that. The idea is that if something needs you 10 minutes to figure out then a comment is worthwhile.

Twey marked this pull request as draft March 28, 2024 20:25

Twey force-pushed the 03-28-Local_view_traits branch 7 times, most recently from 70d1e84 to af96fac Compare April 1, 2024 13:14

Twey marked this pull request as ready for review April 1, 2024 15:01

Twey requested a review from MathieuDutSik April 1, 2024 15:01

Twey commented Apr 1, 2024

View reviewed changes

MathieuDutSik approved these changes Apr 1, 2024

View reviewed changes

Twey force-pushed the 03-28-Local_view_traits branch 2 times, most recently from aa025af to 875e14c Compare April 2, 2024 13:26

MathieuDutSik approved these changes Apr 2, 2024

View reviewed changes

linera-views: add local variants of store traits

6f516e1

Twey force-pushed the 03-28-Local_view_traits branch from 875e14c to 6f516e1 Compare April 2, 2024 15:29

Twey merged commit 85502c9 into linera-io:main Apr 2, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`linera-views`: add local variants of store traits #1843

`linera-views`: add local variants of store traits #1843

Twey commented Mar 28, 2024

Twey Apr 1, 2024

ma2bd Apr 1, 2024

Twey Apr 1, 2024

Twey Apr 2, 2024

MathieuDutSik Apr 2, 2024

Twey Apr 2, 2024 •

edited

Loading

MathieuDutSik left a comment

MathieuDutSik Apr 1, 2024

Twey Apr 2, 2024

MathieuDutSik Apr 1, 2024

Twey Apr 1, 2024

MathieuDutSik Apr 1, 2024

Twey Apr 1, 2024

Twey Apr 2, 2024

Twey Apr 2, 2024

MathieuDutSik Apr 1, 2024

Twey Apr 1, 2024

Twey commented Apr 2, 2024

MathieuDutSik left a comment

MathieuDutSik Apr 2, 2024

linera-views: add local variants of store traits #1843

linera-views: add local variants of store traits #1843

Conversation

Twey commented Mar 28, 2024

Motivation

Proposal

Test Plan

Release Plan

Links

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Twey Apr 2, 2024 • edited Loading

Choose a reason for hiding this comment

MathieuDutSik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Twey commented Apr 2, 2024

MathieuDutSik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

`linera-views`: add local variants of store traits #1843

`linera-views`: add local variants of store traits #1843

Twey Apr 2, 2024 •

edited

Loading