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: Ability to reset subscriber upon out of band seek #662

Merged
merged 9 commits into from
Jun 4, 2021

Conversation

tmdiep
Copy link
Collaborator

@tmdiep tmdiep commented Jun 3, 2021

Prepares for handling the RESET signal from the server, by:

  • Discarding outstanding acks for delivered messages.
  • Waiting for the committer to flush pending commits and receive the acknowledgment from the server.
  • Then resetting subscriber state, including canceling any in-flight client seeks.

Notes:

  • This PR is effectively a no-op as SinglePartitionSubscriber.onSubscriberReset returns false for now.
  • The next PR will fully handle admin seeks by setting InitialSubscribeRequest.initial_location and implement SinglePartitionSubscriber.onSubscriberReset. Omitted to keep the size of the PR down.
  • CommitState.complete had to be refactored to just throw (and not abort) because CommitterImpl.waitUntilEmpty should throw when there are excess commit responses, since the committer will shortly shut down.

@tmdiep tmdiep requested a review from a team as a code owner June 3, 2021 23:24
@product-auto-label product-auto-label bot added the api: pubsublite Issues related to the googleapis/java-pubsublite API. label Jun 3, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jun 3, 2021
@dpcollins-google
Copy link
Collaborator

The next PR will fully handle admin seeks by setting InitialSubscribeRequest.initial_location. Omitted to keep the size of the PR down.

This leaves the client in an indeterminate state where it will drop a bunch of acks but not actually respect the seek on the reconnect. I don't think this is a good state to be in at head. Can you remove the changes to SubscriberImpl from this PR and move them to the next one, so at least if someone happens to (I don't think they will) pick up this intermediate state it will have the same behavior as the existing code (i.e. not reset?)

@tmdiep
Copy link
Collaborator Author

tmdiep commented Jun 4, 2021

Can you remove the changes to SubscriberImpl from this PR and move them to the next one, so at least if someone happens to (I don't think they will) pick up this intermediate state it will have the same behavior as the existing code (i.e. not reset?)

Good point. Instead I just changed SinglePartitionSubscriber.onSubscriberReset to return false, which effectively becomes a no-op.

This will keep the size of the next PR down and avoids re-reviewing many files.

@tmdiep tmdiep changed the title feat: Reset subscriber upon out of band seek feat: Ability to reset subscriber upon out of band seek Jun 4, 2021
@tmdiep tmdiep merged commit 2d89341 into googleapis:master Jun 4, 2021
@tmdiep tmdiep deleted the handle_reset branch June 4, 2021 03:44
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 googleapis/java-pubsublite 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