Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(pubsublite): Committer implementation #3198

Merged
merged 4 commits into from Nov 17, 2020
Merged

Conversation

@tmdiep
Copy link
Contributor

@tmdiep tmdiep commented Nov 11, 2020

Commits desired offsets based on the ack prefix offset. Commits are batched.

Commits desired offsets based on the ack prefix offset. Commits are batched.
@tmdiep
Copy link
Contributor Author

@tmdiep tmdiep commented Nov 11, 2020

WANT_LGTM=@manuelmenzella-google
Other reviewers optional, but feel free to leave comments.

Depends on a number of other PRs to be committed first, mainly #3143.

Calling out potentially controversial behavior of the committer: when Stop() is called, it the stream is left open to process outstanding acks. For example, the user may call Subscriber.Stop() to stop receiving new messages from the server, and continue processing/acking all the outstanding messages. But if they never finish acking all those messages, the committer stream remains open. Perhaps we should have a timer to do a hard shutdown after N minutes? Or expose a Subscriber.Shutdown() to do the hard shutdown (but relies on the user to call this)?

Copy link

@manuelmenzella-google manuelmenzella-google left a comment

Just some nits, feel free to merge. Thanks.

pubsublite/internal/wire/acks.go Outdated Show resolved Hide resolved
pubsublite/internal/wire/committer.go Outdated Show resolved Hide resolved
@tmdiep tmdiep merged commit ecc706b into googleapis:master Nov 17, 2020
3 checks passed
@tmdiep tmdiep deleted the committer branch Nov 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants