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

server side concurrency #49

Merged
merged 21 commits into from
Sep 21, 2018
Merged

server side concurrency #49

merged 21 commits into from
Sep 21, 2018

Conversation

cdevienne
Copy link
Member

@cdevienne cdevienne commented Sep 17, 2018

Implementation of #48.

The first 8 commits refactors the code in order to generate less code and simplify the concurrency implementation.

On side-effect is that a Request struct is now available in the context and provides access to all the request information.

The last commits implements concurrency.

This function takes care of sending the reply after running the handler.
To keep the prometheus metrics, a AfterReply callback was added on the request.
@mdevan mdevan mentioned this pull request Sep 18, 2018
Handlers can optionnally be initialized with a WorkerPool that controls how many concurrent
calls can be handled, and how many pending requests can be queued.
If a request is pending for more than the maxPendingDuration, the scheduler send a SERVERTOOBUSY
to the caller. This way, an overloaded server will always reply something to the client,
which reduces a lot the cases where it gets a timeout error.

The maxPendingDuration should be short enough so that this duration plus the handling
duration remains smaller than the client timeout.

A next version of this system could cancel a running handler if its too long and
send a reply 'server is too slow' to the client
Improve logs capture
The server handler subscription was not unsubscribed at the end of the tests, leading to
multiple responses received.
@cdevienne cdevienne changed the title WIP: server side concurrency server side concurrency Sep 19, 2018
@cdevienne
Copy link
Member Author

I did a review with a colleague, and have a few more things to clean. Stay tuned.

@cdevienne
Copy link
Member Author

@mdevan I think I am done now. I will test the PR in my project this afternoon.

@cdevienne
Copy link
Member Author

Hi @mdevan
It beautifully did the job on my project (xbus).
I would like to merge the PR asap, but I prefer having your feedback first.
Cheers

examples/helloworld/helloworld/helloworld.nrpc.go Outdated Show resolved Hide resolved
examples/metrics_helloworld/helloworld/helloworld.nrpc.go Outdated Show resolved Hide resolved
nrpc.go Outdated Show resolved Hide resolved
nrpc.go Outdated Show resolved Hide resolved
nrpc.go Show resolved Hide resolved
nrpc.go Outdated Show resolved Hide resolved
nrpc.go Outdated Show resolved Hide resolved
nrpc.go Show resolved Hide resolved
nrpc.go Show resolved Hide resolved
nrpc.go Show resolved Hide resolved
@cdevienne
Copy link
Member Author

Thanks @mdevan for the detailed review.
I made some changes based on it and left opened the conversations where I think no patch is needed, so you can prove me wrong (or not :-))
Cheers

@cdevienne cdevienne merged commit 8cd5ee3 into master Sep 21, 2018
@cdevienne cdevienne mentioned this pull request Sep 21, 2018
@cdevienne cdevienne deleted the 48-server-side-concurrency branch October 6, 2020 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants