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

Allow to override prefetch on client and server side #204

Merged
merged 11 commits into from May 12, 2020

Conversation

koldat
Copy link
Contributor

@koldat koldat commented Apr 29, 2020

Default Flowable behavior on Client and Server sending side is to send messages into the queue with length 512. When there is too many concurrent calls and when messages are quite big it can cause OOM. This pull requests allows to override these for such RPC calls.

Client example:

        RxNassStub api = RxNassGrpc.newRxStub(channel)
            .withOption(ClientCalls.CALL_OPTIONS_PREFETCH, 16)
            .withOption(ClientCalls.CALL_OPTIONS_LOW_TIDE, 4);

Server example:

public class TestingService extends NassImplBase {

    @Override
    public int getLowTide(int methodId) {
        return 4;
    }

    @Override
    public int getPrefetch(int methodId) {
        return 8;
    }

@salesforce-cla
Copy link

Thanks for the contribution! Before we can merge this, we need @koldat to sign the Salesforce.com Contributor License Agreement.

Copy link
Collaborator

@rmichela rmichela left a comment

Choose a reason for hiding this comment

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

This is a great contribution! Just a few small changes/questions before merging.

Copy link
Collaborator

@rmichela rmichela left a comment

Choose a reason for hiding this comment

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

I really like where this is going.

Can you refactor the CallOptions and default constants out into their own public ...stub.CallOptions class? That way the ServerCalls class doesn't have to reference the internals of the ClientCalls class. This is also an opportunity to split out a getLowTide(CallOptions options) and getPrefetch(CallOptions options) function.

The last thing it needs is updated documentation explaining how and why to use the new call options.

@rmichela
Copy link
Collaborator

rmichela commented May 7, 2020

Hi @koldat
It's been a while for this PR. Do you have a chance to follow up? I'd really like to release your change. 😁

@koldat
Copy link
Contributor Author

koldat commented May 7, 2020

I have it in my queue, sorry. I will do next week as here are holidays now, ok? But feel free to modify the way you like if you want.

@rmichela
Copy link
Collaborator

rmichela commented May 7, 2020

There is no hurry. 😁

@koldat
Copy link
Contributor Author

koldat commented May 12, 2020

@rmichela, I have pushed another set. please review.

@koldat
Copy link
Contributor Author

koldat commented May 12, 2020

BTW I am trying the same change in Reactor

@koldat
Copy link
Contributor Author

koldat commented May 12, 2020

I have pushed Reactor support as well. Let me know if you like the changes and if you want to squash changesets to a big one.

@rmichela
Copy link
Collaborator

These changes look great! Thanks so much for the contribution.

@koldat
Copy link
Contributor Author

koldat commented May 12, 2020

Do you plan to make a release sometimes? I would like to reference official build instead of custom we are using :)

@rmichela rmichela merged commit be8b740 into salesforce:master May 12, 2020
@rmichela
Copy link
Collaborator

Yes. I'll comment here when released.

@rmichela
Copy link
Collaborator

v1.0.1 is released https://github.com/salesforce/reactive-grpc/releases/tag/v1.0.1

Thank you so much for driving this change!

@koldat
Copy link
Contributor Author

koldat commented May 18, 2020

Thank you very much! Works great :)

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

Successfully merging this pull request may close these issues.

None yet

2 participants