UnixHttpClient.java:57-58: Connection pooling is...

[0pdd]

The puzzle 44-0db77e9f from #44 has to be resolved:

docker-java-api/src/main/java/com/amihaiemil/docker/UnixHttpClient.java

Lines 57 to 58 in 6009d3c

	* @todo #44:30min Connection pooling is currently hardcoded at 10 connections
	* max. Figure out how to make this configurable.

The puzzle was created by George Aristy on 15-Mar-18.

Estimate: 30 minutes, role: DEV.

If you have any technical questions, don't ask me, submit new tickets instead. The task will be "done" when the problem is fixed and the text of the puzzle is removed from the source code. Here is more about PDD and about me.

[0crat]

@amihaiemil/z please, pay attention to this issue

[amihaiemil]

@0crat in

[0crat]

@0crat in (here)

@amihaiemil Job #52 is now in scope, role is DEV

[0crat]

The job #52 assigned to @llorllale/z, here is why. The budget is 30 minutes, see §4. Please, read §8 and §9. If the task is not clear, read this and this.

[llorllale]

@amihaiemil I suppose adding a maxConnections parameter to a ctor of LocalDocker would basically do the trick? Then LocalDocker can pass it on to UnixHttpClient.

I only ask because it doesn't look quite right to me.

Also, do you think 10 is a reasonable default?

[llorllale]

@amihaiemil I'm now wondering if pooling could've been as simple as:

new Pooled(
    10,
    () -> new LocalDocker(new File("/unix/socket"))
)

[amihaiemil]

@llorllale

I'm now wondering if pooling could've been as simple as

No, definetely not; because we certainly needed a pool, so users would always have to do that -- most of them have no idea about connection pooling; or it's not something they think about every day. I also did not think about it before we had the bug.

I suppose adding a maxConnections parameter to a ctor of LocalDocker

I also don't really like this very much. Ideally, the users shouldn't know anything about pooling, the default 10 should be enough for most cases.

I think we can add some sort of decorator, as you suggested :-? So somewhat what we already have + decoration:

Docker docker = new PoolSize(
    25,
    new LocalDocker(...)
);

This PoolSize decorator would only be needed if they want to increase it over the default 10. WDYT?

[llorllale]

@amihaiemil

WDYT?

I'm confused because the first half of your comment seemed to refute my proposal but then the 2nd half seemed to propose (almost) the same thing?

[llorllale]

@amihaiemil actually, I get you now.

[amihaiemil]

@llorllale In your proposal you seemed to hint that we should rollback the pooling we have now and use the decoration exclusively. I said the decoration is nice, but shouldn't be mandatory; so leave the default 10 we have and offer the decorator as an option for changing the pool :D

[llorllale]

@amihaiemil

Docker docker = new PoolSize(
    25,
    new LocalDocker(...)
);

We'd need to change the second arg to be of type Supplier<Docker>, I think, in order to make it work at runtime.

This might now be misleading btw... the semantics would imply a pool size of 25 but it would actually be 25 * DEFAULT, no?

[amihaiemil]

@llorllale hmmm, I see. I also had a look now. I think it would be a lot easier to just offer a ctor with that parameter in LocalDocker. With this Supplier, they would have to instantiate the LocalDocker them selves and also the UnixHttpClient, to pass it the value, right?

[llorllale]

@amihaiemil no they'd just do this

new Pooled(
    2,                                   //multiplies internal pool for a total of 20
    () -> new LocalDocker(new File(""))  //internal pool of 10
)

[llorllale]

@amihaiemil Pooled would maintain pools of Docker instances, not http connections

[amihaiemil]

@llorllale but it would still need access to the underlying connection manager somehow, to see if instance x of Docker has connections left; if not, get the next instance.

The abstraction is too high and besides, it's the job of the HttpClient to manage connections, not of our abstraction layer.

Actually, I think it's better to just make this ctor public and let the user supply their own client if they need tuning. The default 10 connections should be enough, always... and if they want to change the connection pool it means they know what they are doing and probably want to change more things.

Besides making that ctor public, it should also have a Javadoc where we should let the user know that they can do whatever they want but they have to register a unix socket factory as in here -- and that socket factory should not be an anonymous class anymore, but a public one that they can use; you could leave a puzzle for that since you already spent some time on this ticket.

[0pdd]

The puzzle 44-0db77e9f has disappeared from the source code, that's why I closed this issue.

[0crat]

Order was finished: +30 points just awarded to @llorllale/z

[0crat]

The job #52 is now out of scope