#60 Adding a new API which returns the previous entities before bulk upsert #61
Conversation
…upsert. This lets the clients to easily check if an entity has changed as part of an upsert, without making an extra round trip to the service.
buchi-busireddy
requested review from
avinashkolluru,
inespo,
kotharironak and
tim-mwangi
as code owners
Codecov Report
@@ Coverage Diff @@
## main #61 +/- ##
============================================
+ Coverage 53.54% 53.78% +0.23%
- Complexity 329 335 +6
============================================
Files 36 36
Lines 2525 2551 +26
Branches 340 342 +2
============================================
+ Hits 1352 1372 +20
- Misses 1024 1030 +6
Partials 149 149
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
kotharironak
left a comment
kotharironak
left a comment
There was a problem hiding this comment.
lgtm. Had minor comments.
| } | ||
| rpc upsertEntities (Entities) returns (Empty) { | ||
| } | ||
| rpc getAndUpsertEntities (Entities) returns (stream Entity) {} |
There was a problem hiding this comment.
| rpc getAndUpsertEntities (Entities) returns (stream Entity) {} | |
| rpc getAndUpsertEntities (Entities) returns (stream Entity) { | |
| } |
There was a problem hiding this comment.
As rest of the methods are following that pattern.
There was a problem hiding this comment.
I think this is very minor. Will avoid one more commit and full build cycle for this :)
| .map(Entity::toBuilder) | ||
| .map(builder -> builder.setTenantId(tenantId)) | ||
| .map(Entity.Builder::build) | ||
| .forEach(responseObserver::onNext); |
There was a problem hiding this comment.
This will send one document at a time, right? Should we send them in batch?
There was a problem hiding this comment.
Streaming one at a time will allow the client to process it and discard instead of keeping it in memory. i think that's the pattern we should always follow.
Description
See #60 for more details.
Testing
Integration tests are verifying the full logic.
Checklist:
This PR depends on hypertrace/document-store#18 so build will fail until that's merged.