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: Reset Consumer upon out-of-band seek #172

Merged
merged 4 commits into from Aug 11, 2021

Conversation

tmdiep
Copy link
Contributor

@tmdiep tmdiep commented Aug 5, 2021

Resets the Kafka Consumer state to handle admin seeks pushed from the server.

  • SubscriberState was refactored out into SinglePartitionSubscriber. The main motivation was to give each subscriber state its own mutex and update its internal state (for auto-commits) atomically when pulling messages.
    • SingleSubscriptionConsumerImplTest was mostly left as-is to verify that this is a no-op.
  • Added SinglePartitionSubscriber::onSubscriberReset() to reset subscriber state and wait for commits when the RESET signal is received from the server.
    • Undelivered messages are discarded. Further auto-commits are prevented until post-seek messages are received.

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

dpcollins-google commented Aug 8, 2021

I think the majority of your code is correct for the PR. The main thing I'm getting stuck on is what the seek reset behavior should be in the presence of client-initiated seeks. I think its reasonable to say that this is unsupported with a comment in the section here https://github.com/googleapis/java-pubsublite-kafka#about-pubsub-lite-kafka-shim and wherever the equivalent section is in the docs.

@tmdiep
Copy link
Contributor Author

tmdiep commented Aug 10, 2021

I think the majority of your code is correct for the PR. The main thing I'm getting stuck on is what the seek reset behavior should be in the presence of client-initiated seeks. I think its reasonable to say that this is unsupported with a comment in the section here https://github.com/googleapis/java-pubsublite-kafka#about-pubsub-lite-kafka-shim and wherever the equivalent section is in the docs.

I'll note in the Devsite docs for admin seek that mixing use of client-initiated and admin seeks is not recommended, as they would clobber each other.

Also, I amended this PR to disable handling admin seeks when the Consumer auto-commit setting is false, since this could also lead to races:

  1. Client responds to reset signal. Server moves admin seek cursor to commit cursor table.
  2. User-initiated commit overwrites commit cursor.
  3. Subscribe stream breaks before sending any messages.
  4. Subscribe stream will reconnect reading from the user commit cursor, not the admin seek cursor.

@tmdiep tmdiep requested a review from a team as a code owner August 10, 2021 23:57
@tmdiep tmdiep changed the title feat: Handle out-of-band seeks feat: Reset Consumer upon out-of-band seek Aug 11, 2021
@tmdiep tmdiep merged commit 20ae0ba into googleapis:master Aug 11, 2021
@tmdiep tmdiep deleted the reset_subscriber branch August 11, 2021 00:12
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-kafka 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