Review ben stephane #4

smaldini · 2019-02-14T17:07:33Z

No description provided.

src/main/java/reactor/pool/AbstractPool.java

src/main/java/reactor/pool/AffinityPool.java

src/main/java/reactor/pool/AllocationStrategies.java

simonbasle · 2019-02-14T17:24:22Z

src/main/java/reactor/pool/AllocationStrategy.java

@@ -43,6 +41,7 @@
     *
     * @return an ESTIMATED count of how many more resources can currently be allocated
     */
+    //TODO do we need to externalize metrics


It's useful for metrics, but it is not a pure metrics concern. Eg. it is already useful to detect an unbounded strategy and chose the best Queue implementation accordingly.

Yeah, we caught that at the end, but left this in place because it's not clear that it should be used even for that. At issue is that you're creating a pattern that encourages a race condition which you fall afoul of.

The other way to look at this is as a code smell. At this time there are two different parties that need to know the max size of the pool; the allocation strategy and the pool. That means any changes to it probably need to be translated into two locations. This isn't textbook bad, but it does feel like it should be coalesced to a single authority.

The AllocationStrategy was introduced to replace a AtomicIntegerFieldUpdater LIVE + final int maxSize cap. The estimatePermitCount was intended as a way to provide an equivalent to LIVE.get(), with the same limitations in terms of atomicity.

I don't think there is a race condition in the constructor, unless the AllocationStrategy is reused (which it shouldn't be)? Nothing can call getPermit until line 66 initSize.

But maybe the method should be replaced with getPermitCap()? More advanced strategies not purely based on a counter could still return Long.MAX_VALUE to indicate that they have a different mode of granting permits 🤔

Tracked in #7

simonbasle · 2019-02-14T18:00:37Z

src/main/java/reactor/pool/PooledRef.java

@@ -40,6 +35,8 @@
     *
     * @return the poolable
     */
+    //TODO Evaluate if poolable could just be "get" (standard in the JVM reference
+    // containers)


I don't really like the *Reference#get() API style in the JDK, but that's personal preference I guess. I think a more "business wording" style is better. compare Predicate<T>#test vs Function<T,Boolean>#apply.

I recommended the change, but not strongly. My personal preferences go towards the descriptive names, but there's an argument about preferring to be Java-idiomatic. If you care strongly, I'm happy to defer.

Tracked in #11

simonbasle · 2019-02-14T18:01:15Z

src/main/java/reactor/pool/PooledRefMetrics.java

@@ -0,0 +1,60 @@
+package reactor.pool;


see discussion above about re-merging that with PooledRef

As per second round of review, I'm finding what I'm strongly opposed to is to make these metrics "optional" and have instanceof checks in eviction predicates, but it can be less visible (hidden behind one level of indirection, PooledRef#metadata()).

Maybe the term metadata is more accurate than metrics, if we later want to add some feature-specific state?

Tracked in #11

simonbasle · 2019-02-14T18:01:43Z

src/main/java/reactor/pool/QueuePool.java

        for (int i = 0; i < initSize; i++) {
            long start = metricsRecorder.now();
            try {
-                POOLABLE poolable = Objects.requireNonNull(poolConfig.allocator().block(), "allocator returned null in constructor");
+                //FIXME NO!
+                POOLABLE poolable = Objects.requireNonNull(poolConfig.allocator.block(), "allocator returned null in constructor");


see same discussion in AffinityPool ;)

src/main/java/reactor/pool/QueuePool.java

src/test/java/reactor/pool/InMemoryPoolMetrics.java

nebhale · 2019-02-14T18:36:41Z

Just as an FYI, I don't think any of this code (especially the mistaken javadocs and outstanding todos) are anywhere near merging. Its just what we got to last night.

nebhale · 2019-02-14T23:27:35Z

Pool Name (for .name())
Minsize, defaults to max size.

smaldini · 2019-02-14T23:33:45Z

Internals review
Tier-0 pass on Serviceability
Fix Todo list
Fix external jctools use

simonbasle · 2019-02-15T11:00:23Z

src/main/java/reactor/pool/EvictionPredicates.java

@@ -27,7 +24,8 @@
 *
 * @author Simon Baslé
 */
-public final class EvictionPredicates {
+//TODO should be provided by Builder methods directly ?


Mmh ok you're right, obvious examples of direct usage of these predicate in these simple forms are not easy to come by...

I was thinking about a case where you'd want to evict a connection based on age because the configuration used for initialization might periodically change in a non-critical way (ie it is ok to continue using the connection, but preferable to recreate one to accomodate the configuration). But even then, it would probably not be something based purely on age...

Ok, let's do with idle only (since its the most obvious and regular use case), on the builder along with a generic roll-your-own Predicate based option.

simonbasle · 2019-02-15T11:05:20Z

src/main/java/reactor/pool/PoolBuilder.java

+     *
+     * @return this {@link Pool} builder
+     */
+    public PoolBuilder<T> unboundedSize() {


Ok 👍
Would reverting the suffix to a prefix make sense though? sizeUnbounded() vs sizeMax(int)? This way the 2 choices are exposed side by side in autocompletion. (and they would need a @see entry pointing to allocationStrategy, since they overwrite each other).

src/main/java/reactor/pool/PoolBuilder.java

simonbasle · 2019-02-15T11:13:25Z

src/main/java/reactor/pool/PooledRef.java

- * {@link #acquireCount() acquired}, the {@link Pool} may optionally use that information to automatically invalidate
- * the object, and provide a simplified acquire mechanism where the {@link PooledRef} is not directly exposed (see
- * {@link Pool#acquireInScope(Function)} vs {@link Pool#acquire()}).
+ * manually {@link #release()} it to the pool or {@link #invalidate()} it.


Although I expect manual invalidation via invalidate() would largely occur upon detecting failures (like an SQL error code or similar), it can't be excluded that these "metrics" might be useful in triggering a decision to invalidate().

But I think you have a point with the noise aspect. Why not then keep a middle ground:

interface PooledRefMetrics { int acquisitionCount(); long ageMillis(); long idleMillis(); //... } interface PooledRef<T> { release(); //... PooledRefMetrics metrics(); //avoids instanceof }

And have the concrete pooled ref implement both interfaces to limit garbage:

AbstractPooledRef<T> implements PooledRef<T>, PooledRefMetrics { ... }

simonbasle · 2019-02-15T11:16:15Z

src/main/java/reactor/pool/PooledRefMetrics.java

@@ -0,0 +1,60 @@
+package reactor.pool;


As per second round of review, I'm finding what I'm strongly opposed to is to make these metrics "optional" and have instanceof checks in eviction predicates, but it can be less visible (hidden behind one level of indirection, PooledRef#metadata()).

Maybe the term metadata is more accurate than metrics, if we later want to add some feature-specific state?

simonbasle · 2019-02-15T11:23:50Z

src/main/java/reactor/pool/PooledRefMetrics.java

+	 *
+	 * @return the number of times this object has been used by consumers of the pool
+	 */
+	//TODO do we need that for a start


It is easy to compute, we need to mark acquisition to ensure the idleTime() one is correctly reset to 0 anyway, so why not have it (especially if it is less in the user's face)

src/main/java/reactor/pool/PooledRefMetrics.java

simonbasle · 2019-02-15T14:26:12Z

I think I'll integrate most of the changes we agree on here as separate commits on the master branch, as the format of this PR makes it hard to split in discrete chunks (the package reorg kills it for me). I'll also open issues for items still being discussed.

smaldini · 2019-02-15T19:01:31Z

We'll continue on a new PR then - please let us merge the next ones or wait we continue the review next time 👍

Stephane Maldini added 2 commits February 13, 2019 15:32

Suggest one package to rule them all

72a5aef

Implement suggested package change

36cf299

simonbasle requested changes Feb 14, 2019

View reviewed changes

simonbasle requested changes Feb 15, 2019

View reviewed changes

simonbasle closed this Feb 15, 2019

This was referenced Feb 15, 2019

Remove blocking allocations in the pool constructor and add a "warmup" API #5

Closed

Should we keep AllocationStrategy#estimatePermitCount? #7

Closed

Split metadata out of PooledRef #11

Closed

Unadressed review elements to be fleshed out #12

Closed

simonbasle deleted the review-ben-stephane branch April 30, 2020 13:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Review ben stephane #4

Review ben stephane #4

smaldini commented Feb 14, 2019

simonbasle Feb 14, 2019

nebhale Feb 14, 2019

simonbasle Feb 15, 2019

simonbasle Feb 15, 2019

simonbasle Feb 14, 2019

nebhale Feb 14, 2019

simonbasle Feb 15, 2019

simonbasle Feb 14, 2019

simonbasle Feb 15, 2019

simonbasle Feb 15, 2019

simonbasle Feb 14, 2019

nebhale commented Feb 14, 2019

nebhale commented Feb 14, 2019 •

edited

Loading

smaldini commented Feb 14, 2019

simonbasle Feb 15, 2019

simonbasle Feb 15, 2019

simonbasle Feb 15, 2019

simonbasle Feb 15, 2019

simonbasle Feb 15, 2019

simonbasle commented Feb 15, 2019

smaldini commented Feb 15, 2019

Review ben stephane #4

Review ben stephane #4

Conversation

smaldini commented Feb 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nebhale commented Feb 14, 2019

nebhale commented Feb 14, 2019 • edited Loading

smaldini commented Feb 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonbasle commented Feb 15, 2019

smaldini commented Feb 15, 2019

nebhale commented Feb 14, 2019 •

edited

Loading