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: send client id with StreamingPullRequest #58

Merged
merged 7 commits into from Apr 22, 2020

Conversation

@pradn
Copy link
Contributor

@pradn pradn commented Mar 24, 2020

  • 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 Mar 24, 2020
@plamut
Copy link
Contributor

@plamut plamut commented Mar 26, 2020

What about a test? :)
Looks good otherwise.

Loading

@pradn
Copy link
Contributor Author

@pradn pradn commented Mar 26, 2020

Okay, added tests!

Loading

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

@plamut plamut left a comment

LGTM!

Loading

@plamut
Copy link
Contributor

@plamut 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.

Loading

@pradn
Copy link
Contributor Author

@pradn pradn commented Mar 26, 2020

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

Loading

@pradn
Copy link
Contributor Author

@pradn pradn commented Mar 26, 2020

WIll wait for Kamal's review before merging.

Loading

@plamut
Copy link
Contributor

@plamut plamut commented Apr 22, 2020

@pradn Ping, just checking the status of this.

Loading

@gcf-merge-on-green gcf-merge-on-green bot merged commit 9f8acfa into googleapis:master Apr 22, 2020
3 checks passed
Loading
@kamalaboulhosn
Copy link
Contributor

@kamalaboulhosn kamalaboulhosn commented Apr 22, 2020

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

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants