refactor: add common crates #1281

ethowitz · 2022-04-14T20:20:13Z

Description

Add the following crates:
- Top-level syncstorage crate
- syncstorage-db-common
- syncstorage-common
- tokenserver-common
Remove the following dependencies from the database code:
- ApiError
- Actix Web
- HawkIdentifier
Make DbError.kind and ApiError.kind private to simplify API

Things I was unsure about:

I had to remove a lot of the #[cfg(test)] annotations, since anything pinned to the test feature isn't made public to other crates. I don't think this is a big deal but just wanted to mention it

Testing

No change in functionality

Issue(s)

Closes #1275

ethowitz · 2022-04-15T17:53:57Z

I made the opinionated decision to remove syncstorage-rs's dependency on Actix's web::block in favor of tokio's task::spawn_blocking. This prevents us from having to pull Actix in as a dependency for libraries that don't otherwise need it. I'm definitely open to reverting the change if the reviewer thinks it isn't worth it, but I'm trying to separate out the concerns of each of the new crates to reduce the cognitive complexity of how the pieces fit together.

pjenvey · 2022-04-29T23:13:35Z

I had to remove a lot of the #[cfg(test)] annotations, since anything pinned to the test feature isn't made public to other crates. I don't think this is a big deal but just wanted to mention it

FYI at one point the unit tests were in a separate crate (IIRC so they ran on nightly to utilize nightly only async/await 😄 ) and instead of wrapping things in cfg(test) we had a db_test feature they could opt into. I don't think it's worth doing this again though, the unit tests really belong with the syncstorage crate IMO.

I made the opinionated decision to remove syncstorage-rs's dependency on Actix's web::block in favor of tokio's task::spawn_blocking.

It looks like actix_web 4.0's block was changed from actix_threadpool to just be tokio's spawn_blocking underneath so we were going to end up with it anyway. So switching over is fine but with one caveat, that ACTIX_THREADPOOL env var we adjust in settings will no longer be relevant, so we should remove it.

I can't recall tokio spawn_blocking's typical thread pool size, we should double check what it will end up being for us. If it seems too small we can always just load test first and only worry about it if performance looks degraded.

ethowitz · 2022-05-02T14:57:24Z

Looks like the default upper limit on tokio's blocking threads is 512: https://docs.rs/tokio/latest/tokio/runtime/struct.Builder.html#method.max_blocking_threads

I think that should be plenty given that we currently set ACTIX_THREADPOOL to be max(num_cpus * 5, db_pool_max_size)

pjenvey

Just a couple small things.

Thanks for carefully doing this, I really appreciate the git history being preserved.

pjenvey · 2022-05-04T19:38:19Z

syncstorage/src/db/transaction.rs

+                    .await
+                    .map_err(ApiError::from)?;


Looks like this'll continue working:

Suggested change

.await

.map_err(ApiError::from)?;

.await?;

pjenvey · 2022-05-04T19:41:06Z

syncstorage/src/tokenserver/auth/oauth.rs

+            .map_err(|_| TokenserverError::resource_unavailable())?
            .map_err(Into::into)


Suggested change

.map_err(|_| TokenserverError::resource_unavailable())?

.map_err(Into::into)

.map_err(|_| TokenserverError::resource_unavailable())?

pjenvey · 2022-05-04T19:47:35Z

syncstorage/src/web/extractors.rs

@@ -112,6 +117,17 @@ impl BatchBsoBody {
    }
 }

+impl From<BatchBsoBody> for PostCollectionBso {


Leave this for now but I'll note we had a TODO comment somewhere that dissapeared to kill one of these types. I logged #1296 to revisit it later.

pjenvey · 2022-05-04T19:49:05Z

syncstorage/src/web/handlers.rs

-                            .map(|id| (id, "db error".to_owned())),
-                    )
-                }
+                Err(e) if e.is_conflict() || e.is_quota() => return Err(e.into()),


Closes #1275

ethowitz · 2022-05-05T15:51:50Z

Thanks for the review @pjenvey -- this was a long one!

ethowitz · 2022-05-05T16:30:48Z

One thing I just thought of: I changed the database methods to return a DbError instead of an ApiError, and I think that makes the backtrace that gets reported as part of the final ApiError less granular, since the conversion to an ApiError happens in the handler, and it's the conversion from DbError to ApiError that triggers the backtrace collection. Should we add a backtrace to DbError to fix this?

pjenvey · 2022-05-05T16:49:01Z

Yea, good call.

jrconlin

Looks good!

I'm a little concerned about some of the test functions potentially "leaking" into non-test, since those tend to act without safety checks, but I'm fine with flagging them softly for later potential cleanup. Since they seem to be fairly isolated, perhaps we create a "test" trait we apply? 🤷🏻‍♂️

jrconlin · 2022-05-06T16:35:06Z

syncstorage/src/db/mysql/models.rs

-            offset,
-            ids,
-            ..
-        } = params.params;
        let now = self.timestamp().as_i64();


Thank You!

(Sorry, but I kinda hated the source masking that this was doing.)

jrconlin · 2022-05-06T16:48:21Z

syncstorage/src/db/spanner/models.rs

@@ -2128,13 +2104,11 @@ impl<'a> Db<'a> for SpannerDb {
        }
    }

-    #[cfg(test)]
    fn create_collection(&self, name: String) -> DbFuture<'_, i32> {


My only thought about these functions is that they're really not supposed to be used outside of tests. I wonder if it might make sense to eventually move them into a test module rather than have them sprayed around like they currently are. (I also wonder if it might make sense to flag these as test helper functions, like with a comment or something.)

Yeah I think it might be wise to do something with them. Putting them into a separate trait would at least make them slightly harder to use by mistake 🤔 I'll log an issue for this

jrconlin · 2022-05-06T17:34:44Z

syncstorage-db-common/src/lib.rs

@@ -91,6 +80,23 @@ pub struct PoolState {
    pub idle_connections: u32,
 }

+impl From<diesel::r2d2::State> for PoolState {


jrconlin · 2022-05-06T17:39:30Z

syncstorage-db-common/src/util.rs

-            Err(DbErrorKind::Integrity(
-                "Invalid modified i64 (< 0)".to_owned(),
-            ))?;
+            return Err(DbErrorKind::Integrity("Invalid modified i64 (< 0)".to_owned()).into());


ugh, yeah, the previous version was clever. Thanks!

pjenvey · 2022-05-06T19:45:08Z

syncstorage/src/error.rs

+    fn from(db_error: DbError) -> Self {
+        Self {
+            status: db_error.status,
+            backtrace: db_error.backtrace.clone(),


I won't hold up this PR any longer over this but we could avoid this clone -- maybe adding a backtrace2 method that defers to the inner DbError + making the ApiError::backtrace field a private Option<Backtrace>.

thiserror seems to provide a nice way of automatically implementing this for the std::Error::backtrace method but that method's still unstable, unfortunately making many backtrace things like this a bit clunky.

I think we could also use mem::take here, but I'm not sure if that's actually much better than clone (i.e. would memory be allocated for the newly-created default value?)

The answer appears to be yes, as Backtrace::default() just calls Backtrace::new(), which just creates a backtrace :(

ethowitz requested a review from a team April 14, 2022 20:20

ethowitz changed the title ~~refactor: add common crates~~ NOT READY FOR REVIEW Apr 14, 2022

ethowitz marked this pull request as draft April 14, 2022 20:42

ethowitz removed the request for review from a team April 14, 2022 20:42

ethowitz force-pushed the refactor/add-common-crates branch 2 times, most recently from 59dc0e6 to 6018cf7 Compare April 15, 2022 17:51

ethowitz marked this pull request as ready for review April 15, 2022 17:53

ethowitz changed the title ~~NOT READY FOR REVIEW~~ refactor: add common crates Apr 15, 2022

ethowitz requested a review from a team April 15, 2022 17:54

ethowitz force-pushed the refactor/add-common-crates branch from 6018cf7 to 2db1c2f Compare April 25, 2022 20:47

ethowitz force-pushed the refactor/add-common-crates branch from 2db1c2f to de88cc6 Compare May 2, 2022 16:36

pjenvey mentioned this pull request May 3, 2022

chore: to_spanner_value -> into_spanner_value #1301

Merged

pjenvey requested changes May 4, 2022

View reviewed changes

ethowitz requested a review from pjenvey May 5, 2022 15:39

Ethan Donowitz added 4 commits May 5, 2022 11:51

refactor: add common crates

aea7353

Closes #1275

remove actix dependency

ac0a2f2

fmt

fbb2efe

clean up errors

c7f1b7f

ethowitz force-pushed the refactor/add-common-crates branch from 8cb1814 to c7f1b7f Compare May 5, 2022 15:51

pjenvey mentioned this pull request May 5, 2022

feat: adjust ACTIX_THREADPOOL based on Tokenserver max pool size #1287

Closed

add backtrace

ed58328

jrconlin approved these changes May 6, 2022

View reviewed changes

pjenvey approved these changes May 6, 2022

View reviewed changes

ethowitz merged commit a52900f into master May 6, 2022

ethowitz deleted the refactor/add-common-crates branch May 6, 2022 21:42

This was referenced May 9, 2022

chore: tag 0.11.1 #1303

Closed

refactor/remove unnecessary syncserverrequest impl #1295

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: add common crates #1281

refactor: add common crates #1281

ethowitz commented Apr 14, 2022 •

edited

Loading

ethowitz commented Apr 15, 2022

pjenvey commented Apr 29, 2022

ethowitz commented May 2, 2022

pjenvey left a comment

pjenvey May 4, 2022

pjenvey May 4, 2022

pjenvey May 4, 2022

pjenvey May 4, 2022

ethowitz commented May 5, 2022

ethowitz commented May 5, 2022

pjenvey commented May 5, 2022

jrconlin left a comment

jrconlin May 6, 2022

jrconlin May 6, 2022

ethowitz May 6, 2022

jrconlin May 6, 2022

jrconlin May 6, 2022

pjenvey May 6, 2022

ethowitz May 6, 2022

ethowitz May 6, 2022

		.map_err(\|_\| TokenserverError::resource_unavailable())?
		.map_err(Into::into)

refactor: add common crates #1281

refactor: add common crates #1281

Conversation

ethowitz commented Apr 14, 2022 • edited Loading

Description

Testing

Issue(s)

ethowitz commented Apr 15, 2022

pjenvey commented Apr 29, 2022

ethowitz commented May 2, 2022

pjenvey 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

ethowitz commented May 5, 2022

ethowitz commented May 5, 2022

pjenvey commented May 5, 2022

jrconlin 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

ethowitz commented Apr 14, 2022 •

edited

Loading