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

fix(pubsublite): ignore outstanding acks for removed partition subscribers #3597

merged 4 commits into from Jan 28, 2021


Copy link

@tmdiep tmdiep commented Jan 22, 2021

When the assigningSubscriber removes a singlePartitionSubscriber, it should ignore unacked messages when shutting it down (we still attempt to flush the commit cursor for messages that were already acked). This avoids conflicting with the commits from the new subscriber that will be assigned the partition.

Alternatively, we can wait for user code to ack all received messages before acking the new assignment. But this will delay the reassignment operation if user code is taking too long to ack the message.

Copy link
Contributor Author

@tmdiep tmdiep commented Jan 26, 2021

Discussed offline with @dpcollins-google to check that this is the behavior that we want. The Java and Python Lite libraries ignore outstanding acks (or intended to, at least), so we'll do the same in Go (this PR).

It would be better UX to wait for the outstanding unacked messages before acking the assignment, but if user code takes too long to ack the messages, the client would be removed from the assignment set on the server. The partition would be reassigned to another client, and there could potentially be conflicting commits. Considered allowing up to 30s grace period before acking the assignment, but we'll just be consistent with Java and Python.

@tmdiep tmdiep requested a review from as a code owner Jan 28, 2021
@tmdiep tmdiep merged commit eb91f1f into googleapis:master Jan 28, 2021
3 checks passed
@tmdiep tmdiep deleted the assignment_cancel_acks branch Jan 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants