Netty implementation of DockerCmdExecFactory by marcuslinke · Pull Request #397 · docker-java/docker-java

marcuslinke · 2015-12-11T21:47:48Z

This contains an additional (experimental) implementation of DockerCmdExecFactory. It supports http connection hijacking that is needed for passing STDIN to the container which is a long awaited feature (#253).

Currently there are three missing features compared to the current Apache HttpClient based impl:

Connection pooling
Automatic http(s) redirection
Http(s) proxy support

Any feedback is really appreciated!

Conflicts: src/main/java/com/github/dockerjava/api/command/ExecStartCmd.java src/main/java/com/github/dockerjava/api/command/InspectContainerResponse.java src/main/java/com/github/dockerjava/core/command/ExecStartCmdImpl.java

KostyaSha · 2015-12-11T22:33:52Z

src/main/java/com/github/dockerjava/core/command/ExecStartResultCallback.java

remove this header?

KostyaSha · 2015-12-11T22:36:34Z

Resorted imports 😱 After this PR i will try reflect in http://checkstyle.sourceforge.net/config_imports.html#ImportOrder

KostyaSha · 2015-12-11T22:38:57Z

pom.xml

Latest netty stable release 4.0.33.FINAL is missing some features that are needed here. It might be possible to backport it from 4.1.0.Beta7 but as it is an experimental implementation of DockerCmdExecFactory I think its OK for now.

KostyaSha · 2015-12-11T22:55:36Z

Oh, crazy PR :)
Am i right understand that you duplicated all commands for netty?

KostyaSha · 2015-12-11T22:59:51Z

src/main/java/com/github/dockerjava/netty/handler/HttpResponseHandler.java

reformat styling? case intend.

KostyaSha · 2015-12-11T23:05:07Z

Too big for understanding, i propose merge it and test from master. I can try to use new implementation in my reference jenkins plugin.

KostyaSha · 2015-12-11T23:17:47Z

src/main/java/com/github/dockerjava/netty/exec/InfoCmdExec.java

marcuslinke · 2015-12-12T19:38:15Z

@KostyaSha This PR duplicates mostly the Exec parts for the commands only. It doesn't touch any public API, so library users could simply switch to the new netty based DockerCmdExecFactoryImpl by plugging it into the DockerClient and everything should work as before plus it is possible now attach to a running container and pass STDIN to it.

Internally I tried to imitate the JAX-RS API for two reasons:

easier migration from JAX-RS based *Exec classes to netty while coexistence of both implementations
easier implementation of new commands by people not familar with netty but JAX-RS after switching to netty completely

KostyaSha · 2015-12-12T19:39:32Z

I asked from support perspective :) Feel free to merge as soon as you think that it's enough.

marcuslinke · 2015-12-12T19:41:14Z

I'm not sure if I should merge as some important features are still missing. See initial comment please. WDYT?

KostyaSha · 2015-12-12T19:44:34Z

If missing features not breaking existed connection factory, then merge of course.

KostyaSha · 2015-12-12T19:46:51Z

Either i will be unable rework tests/poms.
What probably really need to be checked it's dependencies (hope they are not conflicting).

KostyaSha · 2015-12-14T08:39:12Z

Also please put @Beta annotation on all classes that are not your final implementation.

Netty implementation of DockerCmdExecFactory

Marcus Linke and others added 12 commits November 6, 2015 19:06

Added AHC dependency

2d828ad

start netty implementation (proof of concept)

bc05ff9

Removed some debug output

4a81097

Fix regression in decode algorithm for docker stream

e7f1724

Added basic SSL support

f9c5972

Migrated more command exec implementations and tests

87e398a

WIP

e837b81

Added missing exec implementations as well as appropriate tests

f0dc0c7

Merge branch 'master' into netty

ecc2a19

Conflicts: src/main/java/com/github/dockerjava/api/command/ExecStartCmd.java src/main/java/com/github/dockerjava/api/command/InspectContainerResponse.java src/main/java/com/github/dockerjava/core/command/ExecStartCmdImpl.java

Added copy archive from/to container command for (netty)

a1c7bf5

Added stdin parameter to attach command api, implemented for netty

ca376d6

Removed println() debugging

feb93a4

KostyaSha reviewed Dec 11, 2015
View reviewed changes

src/main/java/com/github/dockerjava/core/command/ExecStartResultCallback.java Outdated

Copy link
Copy Markdown

Member

KostyaSha Dec 11, 2015

Choose a reason for hiding this comment

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

remove this header?

KostyaSha reviewed Dec 11, 2015
View reviewed changes

src/main/java/com/github/dockerjava/netty/handler/HttpResponseHandler.java

Copy link
Copy Markdown

Member

KostyaSha Dec 11, 2015

Choose a reason for hiding this comment

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

reformat styling? case intend.

KostyaSha reviewed Dec 11, 2015
View reviewed changes

src/main/java/com/github/dockerjava/netty/exec/InfoCmdExec.java Outdated

Copy link
Copy Markdown

Member

KostyaSha Dec 11, 2015

Choose a reason for hiding this comment

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

unused?

Code cleanup

8039e18

Added some javadoc

d5dbb5d

marcuslinke added a commit that referenced this pull request Dec 14, 2015

Merge pull request #397 from docker-java/netty

9f77972

Netty implementation of DockerCmdExecFactory

marcuslinke merged commit 9f77972 into master Dec 14, 2015

marcuslinke mentioned this pull request Dec 14, 2015

Support for attaching Stdin when execStart #253

Closed

marcuslinke deleted the netty branch January 12, 2016 22:18

zhaopengme mentioned this pull request Jan 18, 2016

ExecStartCmd failure #424

Closed

KostyaSha mentioned this pull request Jun 22, 2016

docker for mac compatibility #537

Closed

Conversation

marcuslinke commented Dec 11, 2015

Uh oh!

KostyaSha Dec 11, 2015

Choose a reason for hiding this comment

Uh oh!

KostyaSha commented Dec 11, 2015

Uh oh!

KostyaSha Dec 11, 2015

Choose a reason for hiding this comment

Uh oh!

marcuslinke Dec 12, 2015

Choose a reason for hiding this comment

Uh oh!

KostyaSha Dec 12, 2015

Choose a reason for hiding this comment

Uh oh!

KostyaSha commented Dec 11, 2015

Uh oh!

KostyaSha Dec 11, 2015

Choose a reason for hiding this comment

Uh oh!

KostyaSha commented Dec 11, 2015

Uh oh!

KostyaSha Dec 11, 2015

Choose a reason for hiding this comment

Uh oh!

marcuslinke commented Dec 12, 2015

Uh oh!

KostyaSha commented Dec 12, 2015

Uh oh!

marcuslinke commented Dec 12, 2015

Uh oh!

KostyaSha commented Dec 12, 2015

Uh oh!

KostyaSha commented Dec 12, 2015

Uh oh!

KostyaSha commented Dec 14, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants