-
Notifications
You must be signed in to change notification settings - Fork 1
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Failure to commit, while undesirable, shouldn't violate safety.
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, just the lazy me to blame :) I will update this
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.
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 |
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.
good idea
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.
Left some minor comments. Can merge to reduce the number of pending PRs, but will need to finish bits a pieces in TODOs
Use states for the overall candidate as well as for each specific action
764a195
to
283936a
Compare
Future PRs would implement:-