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

Jump to bottom

feat: send client id with StreamingPullRequest #58

Merged
merged 7 commits into from Apr 22, 2020
Merged

feat: send client id with StreamingPullRequest #58

merged 7 commits into from Apr 22, 2020

Conversation

pradn
Copy link
Contributor

@pradn pradn commented Mar 24, 2020 •
edited

  • The client id is created randomly for each StreamingPullManager and is used to establish affinity across stream disconnections/retries.
  • Server-client affinity is important for ordering keys, where the backend tries to send the same keys to the same client.

Fixes #62

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 24, 2020
@plamut
Copy link
Contributor

plamut commented Mar 26, 2020

What about a test? :)
Looks good otherwise.

@pradn
Copy link
Contributor Author

pradn commented Mar 26, 2020

Okay, added tests!

plamut
plamut approved these changes Mar 26, 2020
View reviewed changes
Copy link
Contributor

@plamut plamut left a comment

Choose a reason for hiding this comment

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

LGTM!

@plamut
Copy link
Contributor

plamut commented Mar 26, 2020

Oh, just a sanity check - is it indeed enough to send client ID just in the initial StreamingPullRequest? If not, we should include it in several other StreamingPullRequests the streming pull manager sends.

@pradn
Copy link
Contributor Author

pradn commented Mar 26, 2020

Yes, sending client_id is only required on the first StreamingPullRequest. See the field description in the proto file.

@pradn pradn added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Mar 26, 2020
@pradn
Copy link
Contributor Author

pradn commented Mar 26, 2020

WIll wait for Kamal's review before merging.

@plamut
Copy link
Contributor

plamut commented Apr 22, 2020

@pradn Ping, just checking the status of this.

@plamut plamut added automerge Merge the pull request once unit tests and other checks pass. and removed do not merge Indicates a pull request not ready for merge, due to either quality or timing. labels Apr 22, 2020
@gcf-merge-on-green gcf-merge-on-green bot merged commit 9f8acfa into googleapis:master Apr 22, 2020
@kamalaboulhosn
Copy link
Contributor

Sorry, I had lost track of this. Thanks for adding it!

@release-please release-please bot mentioned this pull request Aug 11, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@plamut plamut plamut approved these changes

@kamalaboulhosn kamalaboulhosn kamalaboulhosn approved these changes

Assignees
No one assigned
Labels
automerge Merge the pull request once unit tests and other checks pass. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Send client id with StreamingPullRequest
4 participants
@pradn @plamut @kamalaboulhosn @googlebot