-
Notifications
You must be signed in to change notification settings - Fork 249
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
Factors redis trigger #2750
Factors redis trigger #2750
Conversation
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 great! I hope the rebase is easy.
use tracing::{instrument, Level}; | ||
|
||
use crate::spin::SpinRedisExecutor; | ||
pub struct RedisTrigger; |
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.
Nit: can we do the _priv: ()
trick here?
}) | ||
type CliArgs = NoCliArgs; | ||
|
||
type InstanceState = (); |
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.
Random thought: I like how type CliArgs = NoCliArgs;
reads much better than type InstanceState = ();
. Perhaps we should consider a NoInstanceState
type which could be used for empty instance state instead of ()
.
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.
fwiw NoCliArgs
is only necessary because of the type CliArgs: clap::Args
bound.
let mut pubsub = client | ||
.get_async_connection() | ||
) -> anyhow::Result<Self> { | ||
let client = Client::open(address)?; |
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.
Is there a reason to open the client connection here instead of in run_listener
?
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.
open
is a lie - its just parsing the address
Signed-off-by: Lann Martin <[email protected]>
Signed-off-by: Lann Martin <[email protected]>
ee57c81
to
381bd80
Compare
@lann I rebased this and am going to merge since we have some other changes that move lots of files around, and it'll be easier on everyone if we just get this one in. |
No description provided.