-
Notifications
You must be signed in to change notification settings - Fork 13.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FLINK-8490] [mesos] Allow custom docker parameters for docker tasks on Mesos #5346
Conversation
Looks great! I need to test that tomorrow in more detail. |
+1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments, should be easy to fix. Overall impressive work!
docs/ops/config.md
Outdated
@@ -508,6 +508,8 @@ May be set to -1 to disable this feature. | |||
|
|||
- `mesos.resourcemanager.tasks.container.volumes`: A comma separated list of `[host_path:]`container_path`[:RO|RW]`. This allows for mounting additional volumes into your container. (**NO DEFAULT**) | |||
|
|||
- `mesos.resourcemanager.tasks.container.docker.parameters`: Custom parameters to be passed into docker run command when using the docker containerizer. Comma separated list of key=value pairs. (**NO DEFAULT**) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 blanks between "containerizer. Comma"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
docs/ops/deployment/mesos.md
Outdated
@@ -264,6 +264,8 @@ May be set to -1 to disable this feature. | |||
|
|||
`mesos.resourcemanager.tasks.container.volumes`: A comma separated list of `[host_path:]`container_path`[:RO|RW]`. This allows for mounting additional volumes into your container. (**NO DEFAULT**) | |||
|
|||
`mesos.resourcemanager.tasks.container.docker.parameters`: Custom parameters to be passed into docker run command when using the docker containerizer. Comma separated list of key=value pairs. (**NO DEFAULT**) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
config.setString(MesosTaskManagerParameters.MESOS_RM_CONTAINER_DOCKER_PARAMETERS, "testKey=testValue"); | ||
|
||
MesosTaskManagerParameters params = MesosTaskManagerParameters.create(config); | ||
assertEquals(params.dockerParameters().get(0).getKey(), "testKey"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe check the size first, same below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a size check.
|
||
for (String dockerParameterSpecification : dockerParameterSpecifications) { | ||
if (!dockerParameterSpecification.trim().isEmpty()) { | ||
String[] match = dockerParameterSpecification.split("=", 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would use split without the limit, i.e., .split("=")
- You already test for length below
- this way you would miss invalid specifications such as key2=val1=val3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some Docker parameters are key=value pairs themselves, so key2=val1=val3 is actually valid specification. I added a comment to clarify this, and included a test case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, thanks for the explanation!
Thanks @joerg84 for your review and comments. I have updated the PR to address your concerns. |
@tillrohrmann Could you take a final look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution @lishim and the reviews @joerg84 and @EronWright. Changes look good to me. Merging this PR.
What is the purpose of the change
Brief change log
Verifying this change
This change added tests and can be verified as follows:
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: noDocumentation