Expose netty connect timeout #725
Conversation
| /** | ||
| * Configure connection timeout in milliseconds | ||
| */ | ||
| public NettyDockerCmdExecFactory withConnectTimeout(Integer connectTimeout) { |
There was a problem hiding this comment.
Jersey has com.github.dockerjava.jaxrs.JerseyDockerCmdExecFactory#withConnectTimeout(Integer connectTimeout) so move method to DockerCmdExecFactory ?
There was a problem hiding this comment.
I'm not sure if we should do that. Consider a new DockerCmdExecFactory implementation where this can't be configured...
There was a problem hiding this comment.
this option is must have for any remote connection and we can remove it any time later.
There was a problem hiding this comment.
Additionally I don't see any advantage to do so. A user is working against a concrete implementation for sure and internally we don't need this option. All these options (connect timeout, read timeout...) were moved from DockerConfig to concrete DockerCmdExecFactory implementation sometimes ago as these are highly implementation specific options.
There was a problem hiding this comment.
Because netty impl is buggy i keep NETTY/JERSEY choose. Connect timeout seems reasonable for any impl. But i can configure it in conditionals on my side of course.
There was a problem hiding this comment.
As its not usual to support multiple implementations I think its OK to do so in your case.
2 participants
@KostyaSha For review.
This change is