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: Add ReassignmentHandler which is notified on client reassignment #886

Merged
merged 9 commits into from
Sep 16, 2021

Conversation

dpcollins-google
Copy link
Collaborator

This enables clients to respond to a reassignment by cancelling outstanding actions and nacking all messages.

Fixes #869

@product-auto-label product-auto-label bot added the api: pubsublite Issues related to the googleapis/java-pubsublite API. label Sep 15, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Sep 15, 2021
*
* <p>Because of the above, as long as reassignment handling is processed quickly, it can be used to
* abort outstanding operations on partitions which are being assigned away from this client, or to
* pre-warm state which will be used by the subscriber handler.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Should we clarify what "subscriber handler" specifically refers to? I was assuming it meant the message receiver function.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.


/**
* A ReassignmentHandler is called any time a new partition assignment is received from the server.
* It will be called with both the previous and new assignments as decided by the backend.
Copy link
Collaborator

@tmdiep tmdiep Sep 15, 2021

Choose a reason for hiding this comment

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

Perhaps also mention that the handler is called after the removed partitions have already been stopped, so subsequent acks/nacks for outstanding messages may be ignored (not 100% sure, but since service shutdown is now async, can we state that they are guaranteed to be ignored?).

Nack was mentioned below, but due to your recent PR, I think it also applies to acks.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This CL specifically adds fencing for nacks and tests for this behavior. The test case added to SinglePartitionSubscriberTest.java confirms that calling stopAsync is sufficient to fence nacks from having any effect. This is a guarantee that nacks are allowed after shutdown- acks may or may not be processed. The wording specifies "will be" which is equivalent to a guarantee.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah this was asking about future message delivery, will fix before submitting.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed, we now wait for subscriber shutdown before acknowledging reassignment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This now applies to acks as well, added to the comment mentioning them.

This enables clients to respond to a reassignment by cancelling outstanding actions and nacking all messages.
@dpcollins-google dpcollins-google force-pushed the client-respond-to-assignment-update branch from 038349a to be1c041 Compare September 16, 2021 13:50
@dpcollins-google dpcollins-google added kokoro:force-run Add this label to force Kokoro to re-run the tests. automerge Merge the pull request once unit tests and other checks pass. labels Sep 16, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 16, 2021
@dpcollins-google dpcollins-google removed the automerge Merge the pull request once unit tests and other checks pass. label Sep 16, 2021
@dpcollins-google dpcollins-google added kokoro:force-run Add this label to force Kokoro to re-run the tests. automerge Merge the pull request once unit tests and other checks pass. labels Sep 16, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 16, 2021
@gcf-merge-on-green gcf-merge-on-green bot merged commit 5bfef8d into master Sep 16, 2021
@gcf-merge-on-green gcf-merge-on-green bot deleted the client-respond-to-assignment-update branch September 16, 2021 14:26
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Sep 16, 2021
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.

Provide support for client partition reassignment handlers
3 participants