[FLINK-7836][Client] specifying node label for flink job to run on yarn #5593
Conversation
|
@tillrohrmann could you review this PR? Thanks! |
tillrohrmann
left a comment
tillrohrmann
left a comment
There was a problem hiding this comment.
Thanks for your contribution @yanghua. I think it's a good addition, but we should add it only as a Yarn specific option and not a general option. Furthermore, it would be good to add a test if possible.
|
|
||
| public void setNodeLabel(String nodeLabel) { | ||
| this.nodeLabel = nodeLabel; | ||
| } |
There was a problem hiding this comment.
We should get rid of this setter pattern. All values should be set at creation time of the AbstractYarnClusterDescriptor.
There was a problem hiding this comment.
@tillrohrmann the setter for nodelabel I followed the example of setZookeeperNamespace and setQueue, it seems that the AbstractYarnClusterDescriptor constructor can not set this field if we do not change it.
There was a problem hiding this comment.
Yes, at some point we should change it in order to make it better maintainable. This will of course not be in the scope of this PR.
docs/ops/cli.md
Outdated
| -z,--zookeeperNamespace <zookeeperNamespace> Namespace to create the | ||
| Zookeeper sub-paths for high | ||
| availability mode | ||
| -nl,--nodeLabel <nodeLabelExpression> Specify YARN node label for |
There was a problem hiding this comment.
I think this should only be a Yarn specific option and not a general option. Mesos and other environments don't necessarily support this.
| } catch (NoSuchMethodException e) { | ||
| LOG.warn("Ignoring node label setting because the version of YARN does not support it"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Could we wrap this call in something like RegisterApplicationMasterResponseReflector?
There was a problem hiding this comment.
Do you mean wrap this code segment to a new method named this?
There was a problem hiding this comment.
@tillrohrmann I know, I just find there is a inner static class ApplicationSubmissionContextReflector, it uses reflection to set fields for ApplicationSubmissionContext such as setApplicationTags. I think add this code segment to this class and wrap it to a method named setApplicationNodeLabel could be better.
What's your opinion?
| private final Method applicationTagsMethod; | ||
| private final Method attemptFailuresValidityIntervalMethod; | ||
| private final Method keepContainersMethod; | ||
| private final Method nodeLabelExpressionMethod; |
There was a problem hiding this comment.
Please mark the field @Nullable.
|
Hi @tillrohrmann should do more work for this PR? |
tillrohrmann
left a comment
tillrohrmann
left a comment
There was a problem hiding this comment.
Changes look good @yanghua. Have you tried these changes out on a Yarn cluster with node labels enabled?
|
@tillrohrmann yes I tested this feature in our inner YARN cluster, both yarn flink session and single job mode. It works fine. |
[FLINK-7836] refactor the code and added some test code [FLINK-7836][Client] mark field : nodeLabelExpressionMethod @nullable This closes apache#5593.
|
hi @tillrohrmann can this PR been merged into master branch, so that we can close it? |
1 similar comment
|
hi @tillrohrmann can this PR been merged into master branch, so that we can close it? |
|
Thanks for the reminder @yanghua. I forgot about it and will merge it now. Thanks again for your contributions. Flink wouldn't be what it is without you! |
[FLINK-7836] refactor the code and added some test code [FLINK-7836][Client] mark field : nodeLabelExpressionMethod @nullable This closes apache#5593.
[FLINK-7836] refactor the code and added some test code [FLINK-7836][Client] mark field : nodeLabelExpressionMethod @nullable This closes apache#5593.
3 participants
What is the purpose of the change
This pull request support flink on yarn to specify yarn node label for yarn flink application both flink session and single job mode
Brief change log
Verifying this change
This change without any test coverage.
Does this pull request potentially affect one of the following parts:
@Public(Evolving): (no)Documentation