You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by mariusz89016 <gi...@git.apache.org> on 2017/02/19 19:30:55 UTC
[GitHub] flink pull request #3356: [FLINK-5253] Remove special treatment of "dynamic ...
GitHub user mariusz89016 opened a pull request:
https://github.com/apache/flink/pull/3356
[FLINK-5253] Remove special treatment of "dynamic properties"
This PR removes special treatment of 'dynamic properties' (aka special way of encoding them as environment variable). Instead, these properties are appended to _flink-conf.yaml_ file.
Link to the issue: https://issues.apache.org/jira/browse/FLINK-5253
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/mariusz89016/flink flink-5253-dynamic-properties
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/3356.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #3356
----
commit 1467b86525d4b2ce07ba9dbe42a5fae7fff1bf22
Author: Mariusz Wojakowski <ma...@allegrogroup.com>
Date: 2017-02-18T14:15:07Z
[FLINK-5253] Remove special treatment of "dynamic properties"
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #3356: [FLINK-5253] Remove special treatment of "dynamic ...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/3356#discussion_r125670830
--- Diff: flink-yarn/src/main/java/org/apache/flink/yarn/AbstractYarnClusterDescriptor.java ---
@@ -711,8 +701,18 @@ public FileVisitResult preVisitDirectory(java.nio.file.Path dir, BasicFileAttrib
LocalResource flinkConf = Records.newRecord(LocalResource.class);
Path remotePathJar =
Utils.setupLocalResource(fs, appId.toString(), flinkJarPath, appMasterJar, fs.getHomeDirectory());
- Path remotePathConf =
- Utils.setupLocalResource(fs, appId.toString(), flinkConfigurationPath, flinkConf, fs.getHomeDirectory());
+
+ final org.apache.flink.configuration.Configuration dynamicProperties = GlobalConfiguration.getDynamicProperties();
--- End diff --
Can't we pass this configuration into the `ClusterDescriptor` differently? What about reading it when creating the descriptor and then passing it as a constructor parameter? Even better, we could read the global configuration, then parse the dynamic properties, adding them to the read config and then creating the descriptor.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #3356: [FLINK-5253] Remove special treatment of "dynamic propert...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on the issue:
https://github.com/apache/flink/pull/3356
I think this PR is still relevant but needs some rebasing onto the latest master @zentol. What we mainly have to do is to use the methods introduced with #4415 to load a configuration with dynamic properties parsed from the command line.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #3356: [FLINK-5253] Remove special treatment of "dynamic propert...
Posted by EronWright <gi...@git.apache.org>.
Github user EronWright commented on the issue:
https://github.com/apache/flink/pull/3356
@tillrohrmann the `dynamicProperties` static field is unfortunate, yes. To eliminate it, we must evaluate the use of the zero-arg `GlobalConfiguration.loadConfiguration()` that is used in many places (`FileInputFormat`, `HadoopUtils`). The field ensures that all those sites see the same configuration.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #3356: [FLINK-5253] Remove special treatment of "dynamic propert...
Posted by EronWright <gi...@git.apache.org>.
Github user EronWright commented on the issue:
https://github.com/apache/flink/pull/3356
Consider using an instance of `Configuration` in place of `Map<String,String>` to store configuration data. `Configuration` provides handy methods for dealing with typed properties and for merging with other collections.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #3356: [FLINK-5253] Remove special treatment of "dynamic propert...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3356
I agree, the CLI should accept dynamic options that are logically added to the configuration. But we still do not need the extra dynamic properties mechanism when setting up the TaskManager and JobManager via YARN.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #3356: [FLINK-5253] Remove special treatment of "dynamic propert...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on the issue:
https://github.com/apache/flink/pull/3356
I think the actual problem is that we use `GlobalConfiguration.loadConfiguration` at far too many places where we should rather pass in a `Configuration` object.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #3356: [FLINK-5253] Remove special treatment of "dynamic ...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/3356#discussion_r125669802
--- Diff: flink-yarn/src/main/java/org/apache/flink/yarn/cli/FlinkYarnSessionCli.java ---
@@ -319,13 +306,7 @@ public AbstractYarnClusterDescriptor createDescriptor(String defaultApplicationN
yarnClusterDescriptor.setTaskManagerSlots(slots);
}
- String[] dynamicProperties = null;
- if (cmd.hasOption(DYNAMIC_PROPERTIES.getOpt())) {
- dynamicProperties = cmd.getOptionValues(DYNAMIC_PROPERTIES.getOpt());
- }
- String dynamicPropertiesEncoded = StringUtils.join(dynamicProperties, YARN_DYNAMIC_PROPERTIES_SEPARATOR);
-
- yarnClusterDescriptor.setDynamicPropertiesEncoded(dynamicPropertiesEncoded);
+ GlobalConfiguration.setDynamicProperties(retrieveDynamicProperties(cmd, DYNAMIC_PROPERTIES));
--- End diff --
Can we get around setting a static field in `GlobalConfiguration`? Why not parsing the dynamic properties and then passing them to the `ClusterDescriptor` at creation time. I guess it would also make sense to do the configuration loading outside of the `ClusterDescriptor`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #3356: [FLINK-5253] Remove special treatment of "dynamic propert...
Posted by EronWright <gi...@git.apache.org>.
Github user EronWright commented on the issue:
https://github.com/apache/flink/pull/3356
@StephanEwen I agree that the YARN AM/TM has no need for dynamic properties since the entire config file is generated per container. But there's a case to be made that dynamic properties on the CLI makes sense for some cluster managers. Imagine a Kubernetes deployment using a docker image with Flink pre-installed & (mostly) pre-configured. It would make sense to pass the JM RPC port (which Kubernetes generates on-the-fly) as a dynamic property, e.g:
```
bin/flink-jobmanager.sh -Djobmanager.rpc.port=$PORT0
```
The mesos jobmanager and taskmanager use this approach for similar reasons.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #3356: [FLINK-5253] Remove special treatment of "dynamic ...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/3356#discussion_r125670308
--- Diff: flink-yarn/src/main/java/org/apache/flink/yarn/cli/FlinkYarnCLI.java ---
@@ -149,13 +148,7 @@ public YarnClusterDescriptorV2 createDescriptor(String defaultApplicationName, C
yarnClusterDescriptor.setJobManagerMemory(jmMemory);
}
- String[] dynamicProperties = null;
- if (cmd.hasOption(DYNAMIC_PROPERTIES.getOpt())) {
- dynamicProperties = cmd.getOptionValues(DYNAMIC_PROPERTIES.getOpt());
- }
- String dynamicPropertiesEncoded = StringUtils.join(dynamicProperties, YARN_DYNAMIC_PROPERTIES_SEPARATOR);
-
- yarnClusterDescriptor.setDynamicPropertiesEncoded(dynamicPropertiesEncoded);
+ GlobalConfiguration.setDynamicProperties(retrieveDynamicProperties(cmd, DYNAMIC_PROPERTIES));
--- End diff --
Can we avoid the static dynamic properties field?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #3356: [FLINK-5253] Remove special treatment of "dynamic propert...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/3356
@tillrohrmann Is this issue still valid after FLINK-7269? (#4415)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #3356: [FLINK-5253] Remove special treatment of "dynamic propert...
Posted by mariusz89016 <gi...@git.apache.org>.
Github user mariusz89016 commented on the issue:
https://github.com/apache/flink/pull/3356
Thanks for the review!
Currently, dynamic properties are set by `GlobalConfiguration.dynamicProperties` instead of saving in cluster descriptor.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #3356: [FLINK-5253] Remove special treatment of "dynamic propert...
Posted by EronWright <gi...@git.apache.org>.
Github user EronWright commented on the issue:
https://github.com/apache/flink/pull/3356
Would you mind having a look at `GlobalConfiguration.dynamicProperties`, which is set by the main method of various Flink processes based on CLI arguments. The `GlobalConfiguration.loadConfiguration` method (which loads the YAML config) treats the dynamic properties as overrides to the config entries. This feature is necessary because `loadConfiguration` is used all over the place.
Just bringing it to your attention in case there's some interplay.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---