allow redis trigger to connect to multiple servers #2242
Conversation
karthik2804
changed the title
crates/redis/src/lib.rs
Outdated
| )); | ||
| } | ||
|
|
||
| let _ = join_all(tasks).await; |
There was a problem hiding this comment.
I think we should possibly exit when one of the listeners exits. thoughts?
There was a problem hiding this comment.
Either exit so that the alarm goes off, or try to restart the relevant listener. But yeah, I don't think we want it continuing in a partial state.
(For what it's worth I think the SQS trigger exits, but that may be because I am lazy.)
There was a problem hiding this comment.
Looking at the SQS trigger futures::future::select_all seems to be what I thought would work
crates/redis/src/lib.rs
Outdated
| )); | ||
| } | ||
|
|
||
| let _ = join_all(tasks).await; |
There was a problem hiding this comment.
Looking at the SQS trigger futures::future::select_all seems to be what I thought would work
karthik2804
force-pushed
the
allow_multiple_redis_server
branch
from
edce6bb
to
44ace99
Compare
There was a problem hiding this comment.
I don't think there are any blockers here, but I feel a little uneasy about some design details (e.g. two lists of addresses, terminology) and there are a couple of wee tidying things. Have a look at the comments and if you go "Ivan you are talking nonsense because" then I am probably okay to approve.
karthik2804
force-pushed
the
allow_multiple_redis_server
branch
2 times, most recently
from
63e4524
to
9af352a
Compare
|
cc: @lann for viz |
karthik2804
changed the title
crates/redis/src/lib.rs
Outdated
|
|
||
| for (_, config) in engine.trigger_configs() { | ||
| channel_components | ||
| let address = config.address.clone().unwrap_or(default_address.clone()); |
There was a problem hiding this comment.
Should we make TriggerMetadata::address optional and only fail if both are missing?
crates/redis/src/lib.rs
Outdated
| let (_, _, rest) = futures::future::select_all(tasks).await; | ||
| drop(rest); |
There was a problem hiding this comment.
Not assigning return values implicitly drops them all.
| let (_, _, rest) = futures::future::select_all(tasks).await; | |
| drop(rest); | |
| futures::future::select_all(tasks).await; |
|
What is required to land this @karthik2804? (Apart from a rebase.) Does something need to be done with Lann's comments? (I'm not sure whether "kinda weird" is saying it needs a rework?) |
|
@lann can you clarify? |
|
"Kinda weird" was not useful feedback, just a vague complaint about preexisting code that I probably approved in the past 😅. |
Signed-off-by: karthik2804 <[email protected]>
Signed-off-by: karthik2804 <[email protected]>
Signed-off-by: karthik2804 <[email protected]>
Signed-off-by: karthik2804 <[email protected]>
itowlson
force-pushed
the
allow_multiple_redis_server
branch
from
f378750
to
473383a
Compare
Adds the ability to connect to multiple Redis servers by optionally specifying an address on the individual trigger config.