[FLINK-4900] flink-master: Allow to deploy TM with container #2703
Conversation
|
Hello :) |
Allows via a setting to deploy a base image on that a task manager runs.
Makman2
force-pushed
the
Makman2/dcos3
branch
from
2cfd825
to
ea0a330
Compare
|
Thanks for the PR! Jenkins CI is currently broken but Travis CI passed. Looks good to me but would be great if you could take a look @EronWright. |
| @@ -489,6 +489,29 @@ | |||
| */ | |||
| public static final String MESOS_RESOURCEMANAGER_TASKS_CPUS = "mesos.resourcemanager.tasks.cpus"; | |||
|
|
|||
| /** | |||
| * The base container image to use for task managers (started via the unified containerizer). | |||
There was a problem hiding this comment.
Please rephrase the comment: "The container image to use for task managers." In other words, it isn't a 'base' image in this context, and numerous containerizers are now supported.
| .setType(Protos.ContainerInfo.Type.DOCKER) | ||
| .setDocker(Protos.ContainerInfo.DockerInfo.newBuilder() | ||
| .setNetwork(Protos.ContainerInfo.DockerInfo.Network.HOST) | ||
| .setForcePullImage(true) |
There was a problem hiding this comment.
"setForcePullImage" is probably not appropriate in production; for dev/test you probably need to use an explicit image tag, not 'latest'.
There was a problem hiding this comment.
Then you can specify the image name with your tag: myimage:devel
But we can make a setting for it which defaults to true (I think force-pulling should be on by default, as it has no side-effects).
There was a problem hiding this comment.
Force pulling does have side-effects, it causes docker to make additional network requests that might not be desirable. This behavior is not typical of other DCOS packages, nor is it consistent between the containerizers. Please turn it off.
| @@ -601,6 +601,54 @@ else if (recoveryMode == HighAvailabilityMode.ZOOKEEPER) { | |||
|
|
|||
| info.setCommand(cmd); | |||
|
|
|||
| // Set base container for task manager if specified in configs. | |||
| String taskManagerContainerName = flinkConfig.getString( | |||
There was a problem hiding this comment.
The variable name (and associated comment) is inaccurate; may I suggest something like tmImageName.
|
Btw shall I use "fixup commits" or fix the commit directly? I prefer the second method, I'm not a real fan of non-atomic commits and commits like "Oops, fixing mistake from before". Though I've read inside the contributions-guidelines that the first method is preferred^^ |
|
It is easier for the review process when you push fixups commits to pull requests. Usually we fixup the commits when merging to master. |
|
I'll merge the changes once the comments have been addressed. |
| .setType(Protos.ContainerInfo.Type.DOCKER) | ||
| .setDocker(Protos.ContainerInfo.DockerInfo.newBuilder() | ||
| .setNetwork(Protos.ContainerInfo.DockerInfo.Network.HOST) | ||
| .setForcePullImage(true) |
There was a problem hiding this comment.
Force pulling does have side-effects, it causes docker to make additional network requests that might not be desirable. This behavior is not typical of other DCOS packages, nor is it consistent between the containerizers. Please turn it off.
|
@mxm LGTM |
Allows via a setting to deploy a base image on that a task manager runs. This closes apache#2703.
Allows via a setting to deploy a base image on that a task manager runs. This closes apache#2703.
Allows via a setting to deploy a base image on that a task manager runs. This closes apache#2703.
Allows via a setting to deploy a base image on that a task manager runs. This closes apache#2703.
Allows via a setting to deploy a base image on that a task manager runs.