-
Notifications
You must be signed in to change notification settings - Fork 11.5k
[indexer-alt][db-agnostic framework][3/n] Apply generic changes to concurrent pipelines #21589
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
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Skipped Deployments
|
411552c
to
abd1a1c
Compare
ab52e9e
to
5cd07c1
Compare
5cd07c1
to
0e91bed
Compare
556a0dd
to
e603603
Compare
d738f28
to
a3b7ed7
Compare
e603603
to
6938cd8
Compare
6938cd8
to
fc737e8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, but needs some clean ups:
- Within all the handler implementations, we should now have to use the
<_ as Store>::Connection
syntax -- just refer to the concrete type. If that's not possible then let's talk about how to make that possible, because it's going to be annoying for builders otherwise. - nit: be consistent about whitespace in Handler impls (empty line after
type Store = Db
). - nit (optional): add a type alias for the connection of a store to avoid having to use the
<_ as Store>::Connection<'_>
syntax everywhere -- this is probably not worth it though if most Handlers don't have to use this syntax.
@@ -112,8 +112,8 @@ pub struct Indexer { | |||
handles: Vec<JoinHandle<()>>, | |||
} | |||
|
|||
// TODO (wlmyng): non-pg store impl<S: TransactionalStore> Indexer<S> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that there should be two impl
blocks for Indexer
-- one with an S: Store
constraint that exposes concurrent_pipeline
and another with an S: TransactionalStore
constraint that exposes sequential_pipeline
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense
-> anyhow::Result<usize>; | ||
async fn commit<'a>( | ||
values: &[Self::Value], | ||
conn: &mut <Self::Store as Store>::Connection<'a>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(optional): I did some digging into why this explicit trait constraining is required, it's just a limitation of the type system: rust-lang/rust#38078.
And it's not a big deal in this case (it's not too difficult to understand what's going on), but there was a nice workaround suggested in the issue, which is to introduce a type alias:
type StoreConnection<'a, S> = <S as Store>::Connection<'a>;
Which can be used here (and elsewhere) to refer to the nested associated connection type for a store associated type.
async fn commit(values: &[Self::Value], conn: &mut db::Connection<'_>) -> Result<usize> { | ||
async fn commit<'a>( | ||
values: &[Self::Value], | ||
conn: &mut <Self::Store as Store>::Connection<'a>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Within handlers, I think it's fine (preferred) to refer to the Connection
directly on the concrete store type:
conn: &mut <Self::Store as Store>::Connection<'a>, | |
conn: &mut Db::Connection<'a>, |
or (if there are two associated Connection
types for Db
):
conn: &mut <Self::Store as Store>::Connection<'a>, | |
conn: &mut db::Connection<'a>, |
But probably if that's the case the better thing to do would be to remove the previously introduced associated type, because it was probably trying to achieve the same thing that the Store
trait is achieving now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other connection type comes from pub type Connection<'p> = PooledConnection<'p, AsyncPgConnection>;
in sui_pg_db
, so a convenience type alias? Even if we were to rename this though, Rust still complains about ambiguous associated type if we try to reference Db::Connection
in the handler. Actually, I don't think sui_pg_db::Connection is associated at all to the Db, the association comes from implementing Store on Db? So rust is complaining that there's a possibility for multiple Store to be implemented for diff Connection. Which is why when we reference db::Connection
the complaint goes away. This works in our favor since we want handler impl to refer to the concrete type anyways
…Connection traits. todos
fc737e8
to
3dbcc6e
Compare
Description
Apply same changes on sequential pipelines (#21537) to concurrent pipelines.
Test plan
How did you test the new or updated feature?
Release notes
Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.
For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.