feat: Talos messenger base #83
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #83 +/- ##
==========================================
- Coverage 57.95% 57.28% -0.68%
==========================================
Files 103 114 +11
Lines 5333 5570 +237
==========================================
+ Hits 3091 3191 +100
- Misses 2242 2379 +137
☔ View full report in Codecov by Sentry. |
| } | ||
|
|
||
| async fn send(&self, version: u64, payload: Self::Payload, additional_data: Self::AdditionalData) -> () { | ||
| info!("[MessengerKafkaPublisher] Publishing message with payload=\n{payload:#?}"); |
There was a problem hiding this comment.
It's good if we could agree on prefix, so that we use it consistently. Somewhere in the code I used format like this: "MessengerKafkaPublisher.send(): ...". Does not matter really just better when there is one format :)
| ))); | ||
| } | ||
| Err(err) => { | ||
| // TODO: GK - what to do on error? Panic? |
There was a problem hiding this comment.
If we panic and do not commit offset then pods will keep restarting and will be stuck in that consume/panic/restart loop.
There was a problem hiding this comment.
Failure to commit, while undesirable, shouldn't violate safety.
There was a problem hiding this comment.
The todo is for the channel. What to do if the channel gives an error. As this is the way I get to know some Kafka action has been processed, and then mark it in suffix.
Realistically the channel would give error when we try to send the message after the receiver was closed, so can I make the assumption that we can ignore the error check for channel.send ?
| } | ||
|
|
||
| #[derive(Debug, Display, Serialize, Deserialize, EnumString, Clone, Eq, PartialEq)] | ||
| pub enum PublishActionType { |
There was a problem hiding this comment.
How about we rename this to represent a list of brokers? If I read the intention correctly this enum wants to list what messaging brokers we can publish to. In that case its no so much of a "...ActionType" anymore.
There was a problem hiding this comment.
Actually can I leave this comment for the next PR please? Some part of it actually would get more generic. Like line 10 - CommitActionType is not used anymore. And my idea around the implementation has become bit more generic since the time these enums where created.
| } | ||
|
|
||
| pub enum MessengerChannelFeedback { | ||
| Error(u64, String), |
There was a problem hiding this comment.
Why not to make simple structs or named variables. Tuple style allows only to show types but names and purposes are very not clear :)
There was a problem hiding this comment.
Agree, just the lazy me to blame :) I will update this
There was a problem hiding this comment.
Sorry, I missed this one while updating the review comments changes. Will update it in the next PR
| todo!() | ||
| } | ||
|
|
||
| // TODO: GK - Elevate this to core suffix |
Use states for the overall candidate as well as for each specific action
gk-kindred
force-pushed
the
feature/talos-messenger
branch
from
764a195
to
283936a
Compare
Future PRs would implement:-