Skip to content

feat: send client id with StreamingPullRequest#58

Merged
gcf-merge-on-green[bot] merged 7 commits intogoogleapis:masterfrom
pradn:add_client_id
Apr 22, 2020
Merged

feat: send client id with StreamingPullRequest#58
gcf-merge-on-green[bot] merged 7 commits intogoogleapis:masterfrom
pradn:add_client_id

Conversation

@pradn
Copy link
Contributor

@pradn pradn commented Mar 24, 2020 •
edited
Loading

  • 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

@pradn pradn requested a review from kamalaboulhosn March 24, 2020 19:36
@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

kamalaboulhosn commented Apr 22, 2020

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

@kamalaboulhosn kamalaboulhosn kamalaboulhosn approved these changes

+1 more reviewer

@plamut plamut plamut approved these changes

Reviewers whose approvals may not affect merge requirements

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