fix: change entity client to not return single #123
Conversation
Codecov Report
@@ Coverage Diff @@
## main #123 +/- ##
============================================
- Coverage 59.77% 59.61% -0.16%
+ Complexity 295 292 -3
============================================
Files 39 39
Lines 2978 2974 -4
Branches 368 368
============================================
- Hits 1780 1773 -7
- Misses 1026 1029 +3
Partials 172 172
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
This comment has been minimized.
This comment has been minimized.
| responseObservers.forEach(updateResult::subscribe); | ||
| EntityDataCachingClient.this | ||
| .createOrUpdateEntity(entityKey, condition) | ||
| .doOnError(error -> log.error("Error upserting entity", error)) |
There was a problem hiding this comment.
one downside of the void return is the error will never make it back - doesn't seem to me like it's worth creating a subscription per call though.
There was a problem hiding this comment.
Can we still throw the exception back, if there was an exception creating/updating the entity?
There was a problem hiding this comment.
no, we've taken off the return of this method, and since it's not blocking this code is executed long after it returns. Abstractly, I prefer the alternative, which I had working - returning a single. But practically, that means that we'll be maintaining a subscription for each call/span for that "eventually" duration (which is currently 15s), just to turn around and take this same action.
There was a problem hiding this comment.
This did remind me though, I need to break the habit of using .doOnError like this - since it doesn't catch it or subscribe to it. The logging still happens, but it'd be followed by a
io.reactivex.rxjava3.exceptions.UndeliverableException
The exception could not be delivered to the consumer because it has already canceled/disposed the flow or the exception has nowhere to go to begin with. Further reading: https://github.com/ReactiveX/RxJava/wiki/What's-different-in-2.0#error-handling |
The following commit illustrates how this is supposed to be handled.
There was a problem hiding this comment.
Didnt quite get this. Isnt doOnError like any other subscriber similar to the blockingSubscribe below?
There was a problem hiding this comment.
it's an operator - internally that does mean a subscription, but the key thing here is that doOnError only acts on the error but doesn't resolve it. You can think of observables as pipelines - if an error makes it to the final subscriber and that final subscriber doesn't handle it, it's considered "undeliverable" and you get the above message. In other words, the doOnError impl was the equivalent of catching, logging and rethrowing - the subscribe version I switched to is just catching and logging.
|
lgtm |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Description
The motivation here is that the previous client API had major performance issues. Given the purpose of this api, high traffic writes, reducing to a simpler API makes sense for the safe of perf.
There were two major issues previously:
This was done as a breaking change, since these only currently have one consumer (enrichment) and that consumer needs to migrate.
Testing
Updated UTs, compiled and tested full system locally.
Checklist: