KvWriteBatch wiring in Sender #184
Conversation
|
@luoyuxia @fresh-borzoni Would appreciate review here. Found this gap when running the kv table example |
luoyuxia
left a comment
luoyuxia
left a comment
There was a problem hiding this comment.
@leekeiabstraction Thanks for the pr. LGTM overall. Could you please add IT for put kv so that the ci can cover it to avoid unexpected code breaks put kv.
|
@luoyuxia ack on integration test. There's still one more bug I'm chasing down on lookup side before writing the integ test. Will raise the PRs when I'm done fixing that lookup bug |
fresh-borzoni
left a comment
fresh-borzoni
left a comment
There was a problem hiding this comment.
@leekeiabstraction Thanks for PR!
Left comment, PTAL
leekeiabstraction
force-pushed
the
sender-wiring
branch
from
050d553 to
54d9748
Compare
|
@fresh-borzoni @luoyuxia Appreciate the review. Addressed your comments. PTAL |
| ) -> Result<WriteRequest> { | ||
| let first_batch = &request_batches.first().unwrap().write_batch; | ||
|
|
||
| let request = match first_batch { |
There was a problem hiding this comment.
Should we symmetrically validate ArrowLog as well?
There was a problem hiding this comment.
Ah, I see, It's a bit hidden invariant at work, I think we may assume that all the batches by table_id are of the same type, so first batch is fine here.
fresh-borzoni
left a comment
fresh-borzoni
left a comment
There was a problem hiding this comment.
@leekeiabstraction Thanks for addressing, looks solid.
Left comment about ArrowLog, but as I see Java does the same.
| table_id, | ||
| table_buckets, | ||
| records_by_bucket, | ||
| response, |
There was a problem hiding this comment.
cool macro, but i wonder if this can become an async trait
4 participants
Purpose
Linked issue: close #183
Wire up KvWriteBatch within Sender
Tested using example Kv table in: PR181