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

fix #51 Expose PoolConfig, rework PoolBuilder with factory #55

Merged
merged 4 commits into from
Jul 12, 2019

Conversation

simonbasle
Copy link
Member

This commit heavily reworks how the PoolBuilder is producing a PoolConfig
(internally) and then a concrete Pool instance.

The goal is to open the builder for cases where implementations would be
provided by external projects (eg. a netty event-loop aware implementation
for a project that pools netty connections).

One can pass a factory Function to the builder to create such a custom
pool, possibly also switching the builder to a custom type of configuration
beforehand. As a result, the DefaultPoolConfig has now been extracted as
PoolConfig and made public.

As a shorthand for the build(Function) that produces reactor-pool vanilla
implementations, we now expose lifo() and
fifo(), which are now the two out-of-the-box provided flavors.

@codecov-io
Copy link

codecov-io commented Jun 14, 2019

Codecov Report

Merging #55 into master will decrease coverage by 1.01%.
The diff coverage is 86.81%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #55      +/-   ##
============================================
- Coverage     84.68%   83.66%   -1.02%     
- Complexity      113      124      +11     
============================================
  Files            11       12       +1     
  Lines           457      502      +45     
  Branches         67       67              
============================================
+ Hits            387      420      +33     
- Misses           44       55      +11     
- Partials         26       27       +1
Impacted Files Coverage Δ Complexity Δ
src/main/java/reactor/pool/AbstractPool.java 89.1% <100%> (-1.16%) 16 <5> (ø)
src/main/java/reactor/pool/PoolBuilder.java 95.31% <100%> (+1.31%) 25 <8> (+1) ⬆️
src/main/java/reactor/pool/SimpleLifoPool.java 91.17% <100%> (ø) 16 <4> (ø) ⬇️
src/main/java/reactor/pool/SimpleFifoPool.java 82.35% <100%> (ø) 14 <3> (ø) ⬇️
src/main/java/reactor/pool/DefaultPoolConfig.java 76.74% <76.74%> (ø) 11 <11> (?)
src/main/java/reactor/pool/SimplePool.java 75.75% <81.81%> (-1.22%) 25 <1> (-1)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9a45228...c15fd42. Read the comment docs.

@simonbasle
Copy link
Member Author

@nebhale can you have a look as well? good preview of the capacity for extending the builder with a custom configuration type is displayed in the test: https://github.com/reactor/reactor-pool/pull/55/files#diff-69a164d236f7941f6d8810aa3ebc0498R112

This commit heavily reworks how the PoolBuilder is producing
a PoolConfig (internally) and then a concrete Pool instance.

The goal is to open the builder for cases where implementations
would be provided by external projects (eg. a netty event-loop
aware implementation for a project that pools netty connections).

One can pass a factory Function to the builder to create such
a custom pool, possibly also switching the builder to a custom
type of configuration beforehand. As a result, the
DefaultPoolConfig has now been extracted as PoolConfig and made
public.

As a shorthand for the `build(Function)` that produces
reactor-pool vanilla implementations, we now expose `lifo()` and
`fifo()`, which are now the two out-of-the-box provided flavors.
@simonbasle simonbasle force-pushed the 51-poolBuilderAndConfigCustomization branch from e04eda5 to e525c3f Compare July 3, 2019 14:42
@simonbasle
Copy link
Member Author

@violetagg @smaldini we could change the fifo() and lifo() builder methods to return an InstrumentedPool, so that the most specific public type can be used without instanceof/cast, wdyt?

PoolConfig is now an interface, with a DefaultPoolConfig implementation
that external pools can extend.

acquisitionScheduler denotes a thread-determinism, but not all pool
implementations are required to have such determinism. If they do, and
the scheduler is set (to something other than immediate), then they MUST
use this Scheduler.
Copy link

@bsideup bsideup left a comment

Choose a reason for hiding this comment

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

The implementation looks good 👍
Although I'm lacking some background of the change and would suggest to verify the change with the ones who requested it (reactor-netty?)

@simonbasle simonbasle merged commit 740b55b into master Jul 12, 2019
@simonbasle simonbasle deleted the 51-poolBuilderAndConfigCustomization branch July 12, 2019 13:52
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.

None yet

3 participants