Adding a Flux based subscribeToEvents method #1598
Conversation
Signed-off-by: Artur Ciocanu <[email protected]>
Signed-off-by: Artur Ciocanu <[email protected]>
Signed-off-by: Artur Ciocanu <[email protected]>
Signed-off-by: Artur Ciocanu <[email protected]>
Signed-off-by: Artur Ciocanu <[email protected]>
Signed-off-by: Artur Ciocanu <[email protected]>
Signed-off-by: Artur Ciocanu <[email protected]>
Signed-off-by: Artur Ciocanu <[email protected]>
|
@dapr/approvers-java-sdk and @dapr/maintainers-java-sdk could you please take a look at this PR. It tries to add a more ergonomic API for Please take a look and let me know your thoughts. @javier-aliaga as usual, I would be interested to hear your thoughts. @salaboy I am wondering if we can pause the promotion of |
Signed-off-by: Artur Ciocanu <[email protected]>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1598 +/- ##
============================================
+ Coverage 76.91% 78.63% +1.71%
- Complexity 1592 1950 +358
============================================
Files 145 217 +72
Lines 4843 5943 +1100
Branches 562 664 +102
============================================
+ Hits 3725 4673 +948
- Misses 821 929 +108
- Partials 297 341 +44 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| * @return An active subscription. | ||
| * @deprecated Use {@link #subscribeToEvents(String, String, TypeRef)} instead for a more reactive approach. | ||
| */ | ||
| @Deprecated |
There was a problem hiding this comment.
@artur-ciocanu do we need to deprecate the old one? I haven't had time to try this out, but it feels like without flux, this will not be efficient, but does it work as expected?
|
@artur-ciocanu yeah.. totally, we need the Flux approach to get this consistently. The promotion was needed because these APIs are already stable in the Dapr Sidecar, so I would vote for having the Flux approach in the main Dapr Client |
| @@ -0,0 +1,446 @@ | |||
| /* | |||
| * Copyright 2024 The Dapr Authors | |||
| @@ -0,0 +1,196 @@ | |||
| /* | |||
| * Copyright 2024 The Dapr Authors | |||
| try { | ||
| requestStream.onCompleted(); | ||
| } catch (Exception e) { | ||
| // Ignore cleanup errors |
There was a problem hiding this comment.
Can we log at least in debug mode?
| var ack = buildSuccessAck(id); | ||
|
|
||
| requestStream.onNext(ack); | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
I think this catch is too agresive, probably we should treat deserialize errors in a different way as sink errors?
| } | ||
| } catch (Exception ex) { | ||
| // If we can't send ack, propagate the error | ||
| sink.error(DaprException.propagate(ex)); |
There was a problem hiding this comment.
if this fails the original exception on line 112 is lost, maybe this chained catches can we rewritten to be sequentials?
ex = interlaNext
if ex != null {
retry()
}
Or something similar
3 participants
Description
This is PR is an attempt to create a new version of
subscribeToEvents()that aligns better with Project Reactor APIs. It is really weird to have Project Reactor APIs mixed with callbacks likeSubscriptionListener.Issue reference
We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.
Please reference the issue this PR will close: N/A
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: