Skip to content

Conversation

@lishim
Copy link
Contributor

@lishim lishim commented Jun 30, 2018

What is the purpose of the change

  • This pull request introduces an option for Mesos executor to forcefully pull Docker images

Brief change log

  • A new parameter mesos.resourcemanager.tasks.container.docker.force-pull-image was introduced

Verifying this change

This change added tests and can be verified as follows:

  • Added unit tests that verify correctness of behavior of new parameter
  • Manually verified behavior by running Flink in Mesos with Docker containerizer and 2 task managers. Confirmed that when a task started on a node where the task image already resided, an existing cached image was used when mesos.resourcemanager.tasks.container.docker.force-pull-image was left out or set to false. Confirmed that fresh image was pulled and used over a cached image when mesos.resourcemanager.tasks.container.docker.force-pull-image was set to true.

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): no
  • The public API, i.e., is any changed class annotated with @Public(Evolving): no
  • The serializers: no
  • The runtime per-record code paths (performance sensitive): no
  • Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: yes
  • The S3 file system connector: no

Documentation

  • Does this pull request introduce a new feature? yes
  • If yes, how is the feature documented? docs

Copy link
Contributor

@tillrohrmann tillrohrmann left a comment

Choose a reason for hiding this comment

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

Thanks for opening this PR @lishim. I had a comment concerning the name of the config option. Apart from that, the PR looks good and can be merged after addressing the comment.

" Comma separated list of \"key=value\" pairs. The \"value\" may contain '='.");

public static final ConfigOption<Boolean> MESOS_RM_CONTAINER_DOCKER_FORCE_PULL_IMAGE =
key("mesos.resourcemanager.tasks.container.docker.forcePullImage")
Copy link
Contributor

Choose a reason for hiding this comment

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

In order to make it consistent with other configuration options, we should name this config option force-pull-image.

@lishim lishim force-pushed the feature/forcePullImage branch from 2e6274b to b3264f4 Compare July 1, 2018 16:32
@lishim lishim force-pushed the feature/forcePullImage branch from b3264f4 to a5eb20b Compare July 2, 2018 15:30
@lishim
Copy link
Contributor Author

lishim commented Jul 2, 2018

Hi @tillrohrmann. Thank you for your comment. I have updated the code to conform to other configuration options.

Copy link
Contributor

@tillrohrmann tillrohrmann left a comment

Choose a reason for hiding this comment

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

Thanks for addressing my comments @lishim. LGTM. Merging this PR.

tillrohrmann pushed a commit to tillrohrmann/flink that referenced this pull request Jul 3, 2018
@asfgit asfgit closed this in b230bf0 Jul 4, 2018
Xeli pushed a commit to Xeli/flink that referenced this pull request Jul 17, 2018
sampathBhat pushed a commit to sampathBhat/flink that referenced this pull request Jul 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants