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

JerseyClientBuilder can create rx-capable client #1721

Merged
merged 2 commits into from Sep 21, 2016

Conversation

Projects
None yet
4 participants
@nickbabcock
Contributor

nickbabcock commented Sep 7, 2016

Jersey allows for clients to have a reactive api if created appropriately:
https://jersey.java.net/documentation/latest/rx-client.html

This functionality for creating an rx-capable client is not currently in
dropwizard, which may lead users down the wrong path. They'll try wrapping
the dropwizard configured client like so (Java8 example):

RxCompletionStage.from(client);

This is very bad because this will not use the executor carefully crafted
by dropwizard, but instead use ForkJoinPool#commonPool, which is, by
default, created to only have as many threads as there are numbers of CPUs - 1.
The end result is that many requests will end up blocking, the exact
opposite of what a user may think by using this API.

This commit introduces an rx-capable client that wraps the dropwizard
client with the dropwizard created executor so that both async and rx
methods can take advantage of the same thread pool, something that is more
intuitive than the current behavior.

Jersey supports four different reactive libraries and this commit works
with any of them and all future versions.

An alternative implementation would be to expose the executor that
dropwizard cofigures, but I find the approach taken in this commit better.

nickbabcock added some commits Sep 3, 2016

JerseyClientBuilder can create rx-capable client
Jersey allows for clients to have a reactive api if created appropriately:
https://jersey.java.net/documentation/latest/rx-client.html

This functionality for creating an rx-capable client is not currently in
dropwizard, which may lead users down the wrong path. They'll try wrapping
the dropwizard configured client like so (Java8 example):

```java
RxCompletionStage.from(client);
```

This is very bad because this will not use the executor carefully crafted
by dropwizard, but instead use `ForkJoinPool#commonPool`, which is, by
default, created to only have as many threads as there are numbers of CPUs
- 1. The end result is that many requests will end up blocking, the exact
opposite of what a user may think by using this API.

This commit introduces an rx-capable client that wraps the dropwizard
client with the dropwizard created executor so that both `async` and `rx`
methods can take advantage of the same thread pool, something that is more
intuitive than the current behavior.

Jersey supports four different reactive libraries and this commit works
with any of them and all future versions.

An alternative implementation would be to expose the executor that
dropwizard cofigures, but I find the approach taken in this commit better.
@coveralls

This comment has been minimized.

coveralls commented Sep 7, 2016

Coverage Status

Coverage increased (+0.003%) to 82.296% when pulling ad45005 on nickbabcock:rx-client into 6e48972 on dropwizard:master.

@jplock

This comment has been minimized.

Member

jplock commented Sep 7, 2016

This looks good to me. Should we update the release notes and provide an example in the docs for this?

@nickbabcock nickbabcock force-pushed the nickbabcock:rx-client branch from ee913fb to b96944f Sep 7, 2016

@nickbabcock

This comment has been minimized.

Contributor

nickbabcock commented Sep 7, 2016

Ask and you shall receive 😉

I'll update the release notes out-of-band once this PR is merged

EDIT: oof looks like my documentation commit was on a machine without ntp, sorry for the date drift, if you need me to recommit, let me know.

@coveralls

This comment has been minimized.

coveralls commented Sep 7, 2016

Coverage Status

Coverage increased (+0.003%) to 82.298% when pulling b96944f on nickbabcock:rx-client into 7295c5f on dropwizard:master.

@coveralls

This comment has been minimized.

coveralls commented Sep 7, 2016

Coverage Status

Coverage increased (+0.003%) to 82.298% when pulling b96944f on nickbabcock:rx-client into 7295c5f on dropwizard:master.

@arteam arteam merged commit d882afd into dropwizard:master Sep 21, 2016

@jplock jplock added the improvement label Sep 21, 2016

@jplock jplock added this to the 1.1.0 milestone Sep 21, 2016

@nickbabcock

This comment has been minimized.

Contributor

nickbabcock commented Sep 21, 2016

Updated release 1.1 notes with this: 78dff9b

@arteam

This comment has been minimized.

Member

arteam commented Sep 21, 2016

Awesome, thanks!

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