-
Notifications
You must be signed in to change notification settings - Fork 119
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
issue #292 - discard support #293
Conversation
Thanks for the contribution! Unfortunately we can't verify the commit author(s): Kirill Rodionov <k***@g***.com>. One possible solution is to add that email to your GitHub account. Alternatively you can change your commits to another email and force push the change. After getting your commits associated with your GitHub account, sign the Salesforce.com Contributor License Agreement and this Pull Request will be revalidated. |
851f581
to
77dabe8
Compare
Thanks for the contribution! Before we can merge this, we need @bruto1 to sign the Salesforce.com Contributor License Agreement. |
77dabe8
to
25eb33d
Compare
25eb33d
to
0601bfc
Compare
can anyone else take the time to review this, please? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First, thanks for the pr. These changes and tests appear fine to me.
However, I do not wish to be the only approval for this fix since I'm not at all familiar with this part of the reactive-grpc codebase. I was not involved in the development of reactive-grpc and was recently added as a committer after helping with reactivex 2->3 transition.
This looks good. Why wouldn't we implement something similar for RxJava? Is this a Reactor specific issue? |
@nikolay-pshenichny Can you please take a look? |
I don't know first thing about RxJava, @rmichela - sry :) Would've changed both impls otherwise |
please review, @rmichela