-
Notifications
You must be signed in to change notification settings - Fork 19
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: bi-directional streaming map #197
Conversation
Signed-off-by: Sidhant Kohli <[email protected]>
Signed-off-by: Sidhant Kohli <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #197 +/- ##
==========================================
- Coverage 94.24% 94.20% -0.04%
==========================================
Files 56 55 -1
Lines 2259 2314 +55
Branches 145 154 +9
==========================================
+ Hits 2129 2180 +51
- Misses 96 98 +2
- Partials 34 36 +2 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Sidhant Kohli <[email protected]>
Signed-off-by: Sidhant Kohli <[email protected]>
Signed-off-by: Sidhant Kohli <[email protected]>
# If the message is an exception, we raise the exception | ||
if isinstance(msg, BaseException): | ||
await handle_async_error(context, msg) | ||
return |
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.
We are not really returning anything right? But it should ideally be AsyncIterable[map_pb2.MapResponse]
correct?
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.
Correct, but the error handler would have sent a context error back, hence empty return for exit here.
d = Datum( | ||
keys=list(request.request.keys), | ||
value=request.request.value, | ||
event_time=request.request.event_time.ToDatetime(), | ||
watermark=request.request.watermark.ToDatetime(), | ||
headers=dict(request.request.headers), | ||
) |
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.
Do we need to have this part inside the try except?
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 there is a type issue, good to catch that mostly
Signed-off-by: Sidhant Kohli <[email protected]>
Signed-off-by: Sidhant Kohli <[email protected]>
E2E test
|
Kindly explain what this PR does.