feat: add batch reading for Jetstream #1678
Conversation
Signed-off-by: Sidhant Kohli <sidhant.kohli@gmail.com>
| if err = msgs.Error(); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
we cannot do this; we will have to return partial read messages, or else it is a data loss scenario. Perhaps you can return err at the end.
There was a problem hiding this comment.
I can return the partial messages read along with the error.
Though one question, in case we return no messages, they won't be acked by us as well right? And ideally the whole batch should be retried in the next read?
| for _, msg := range msgs { | ||
|
|
||
| // Wait until the batch processing is done | ||
| <-msgs.Done() |
There was a problem hiding this comment.
Be very cautious on this async fetch call, I don't trust the reliability based on our past experience.
There was a problem hiding this comment.
@kohlisid, can you check whether it uses Fetch behind the scenes?
There was a problem hiding this comment.
It does not use Fetch call directly. Both of them have different ways of collating and reporting the data.
@whynowy Any specific areas that we can verify to ensure the scenarios you are mentioning?
vigith
changed the title
|
what's the plan for this PR? |
|
@vigith I was thinking to get this done along with the new Jetstream API changes. With the changes in functionality from their side regarding the same. |
close this PR then? |
Fixes #1675