-
Notifications
You must be signed in to change notification settings - Fork 454
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
[aggregator] Add M3Msg client and server for M3Aggregator #2171
Conversation
union *encoding.UnaggregatedMessageUnion, | ||
msg consumer.Message, | ||
) error { | ||
defer msg.Ack() |
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.
Are all of the below error cases non-retryable? If we unconditionally ack we'll never retry from the coordinator client to the aggregator right?
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.
I believe we probably should avoid retries on this, if aggregator gets overloaded the amount of buffering/retries will probably quickly overwhelm the coordinators.
Codecov Report
@@ Coverage Diff @@
## master #2171 +/- ##
======================================
Coverage 71.9% 71.9%
======================================
Files 1024 1024
Lines 89726 89726
======================================
Hits 64581 64581
Misses 20827 20827
Partials 4318 4318
Continue to review full report at Codecov.
|
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 a few comments.
Changes overall LGTM but I don't have any context on things like why the legacy protocol has to watch for placements while the new m3msg protocol does not.
What this PR does / why we need it:
This adds the ability for users to use M3Msg to write and consume.
Special notes for your reviewer:
Does this PR introduce a user-facing and/or backwards incompatible change?:
Does this PR require updating code package or user-facing documentation?: