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 tmdiep requested a review from a team as a code owner November 11, 2020 21:49
@product-auto-label product-auto-label bot added the api: pubsublite Issues related to the Pub/Sub Lite API. label Nov 11, 2020
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Nov 11, 2020
@tmdiep tmdiep added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Nov 11, 2020
@tmdiep
Copy link
Contributor Author

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Nov 17, 2020
@tmdiep tmdiep merged commit ecc706b into googleapis:master Nov 17, 2020
@tmdiep tmdiep deleted the committer branch November 17, 2020 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: pubsublite Issues related to the Pub/Sub Lite API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants