You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by StefanRRichter <gi...@git.apache.org> on 2016/07/14 17:05:45 UTC
[GitHub] flink pull request #2249: 4166 zookeeper namespaces
GitHub user StefanRRichter opened a pull request:
https://github.com/apache/flink/pull/2249
4166 zookeeper namespaces
Thanks for contributing to Apache Flink. Before you open your pull request, please take the following check list into consideration.
If your changes take all of the items into account, feel free to open your pull request. For more information and/or questions please refer to the [How To Contribute guide](http://flink.apache.org/how-to-contribute.html).
In addition to going through the list, please provide a meaningful description of your changes.
- [ ] General
- The pull request references the related JIRA issue ("[FLINK-XXX] Jira title text")
- The pull request addresses only one issue
- Each commit in the PR has a meaningful commit message (including the JIRA id)
- [ ] Documentation
- Documentation has been added for new functionality
- Old documentation affected by the pull request has been updated
- JavaDoc for public methods has been added
- [ ] Tests & Build
- Functionality added by the pull request is covered by tests
- `mvn clean verify` has been executed successfully locally or a Travis build has passed
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/StefanRRichter/flink 4166-zookeeper-namespaces
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/2249.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 #2249
----
commit 6e418c9ee6004cd13d4bfde158ff0b56cb0136aa
Author: Stefan Richter <s....@data-artisans.com>
Date: 2016-07-14T16:21:40Z
[FLINK-4166] [Distributed Coordination] zookeeper namespaces (cli parameter -z)
commit f343da4042a02aa86b5bde81f37d13200a081b41
Author: Stefan Richter <s....@data-artisans.com>
Date: 2016-07-14T17:01:15Z
[FLINK-4166] [Distributed Coordination] doku: zookeeper namespaces (cli parameter -z)
----
---
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 #2249: 4166 zookeeper namespaces
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2249#discussion_r70884431
--- Diff: flink-yarn/src/main/java/org/apache/flink/yarn/YarnApplicationMasterRunner.java ---
@@ -72,26 +68,36 @@
* This class is the executable entry point for the YARN application master.
* It starts actor system and the actors for {@link org.apache.flink.runtime.jobmanager.JobManager}
* and {@link YarnFlinkResourceManager}.
- *
+ * <p>
--- End diff --
Of the 96 additions made a grand total of 6 are actually related to this PR. Please limit your auto-formatting. I'm not a fan of checking 15x the number of lines necessary.
---
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 #2249: 4166 zookeeper namespaces
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2249#discussion_r70885172
--- Diff: flink-yarn/src/main/java/org/apache/flink/yarn/AbstractYarnClusterDescriptor.java ---
@@ -545,6 +551,19 @@ protected YarnClusterClient deployInternal() throws Exception {
// Set-up ApplicationSubmissionContext for the application
ApplicationSubmissionContext appContext = yarnApplication.getApplicationSubmissionContext();
+ final ApplicationId appId = appContext.getApplicationId();
--- End diff --
since this is only used in the `if` block it should be moved in there.
---
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 #2249: 4166 zookeeper namespaces
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2249#discussion_r70883754
--- Diff: flink-yarn/src/main/java/org/apache/flink/yarn/AbstractYarnClusterDescriptor.java ---
@@ -545,6 +551,19 @@ protected YarnClusterClient deployInternal() throws Exception {
// Set-up ApplicationSubmissionContext for the application
ApplicationSubmissionContext appContext = yarnApplication.getApplicationSubmissionContext();
+ final ApplicationId appId = appContext.getApplicationId();
+
+ // ------------------ Add Zookeeper namespace to local flinkConfiguraton ------
+ String zkNamespace = getZookeeperNamespace();
+ // no user specified cli argument for namespace?
+ if(zkNamespace == null || zkNamespace.isEmpty()) {
+ // namespace defined in config? else use applicationId as default.
+ zkNamespace = flinkConfiguration.getString(ConfigConstants.ZOOKEEPER_NAMESPACE_KEY, String.valueOf(appId));
+ setZookeeperNamespace(zkNamespace);
+ }
+
+ flinkConfiguration.setString(ConfigConstants.ZOOKEEPER_NAMESPACE_KEY, zkNamespace);
--- End diff --
this should be in an `else` block, otherwise we may set a property even though it is already set.
---
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 #2249: 4166 zookeeper namespaces
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2249#discussion_r70876628
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/util/ZooKeeperUtils.java ---
@@ -285,6 +291,18 @@ public static ZooKeeperCheckpointIDCounter createCheckpointIDCounter(
}
}
+ private static String generateZookeeperPath(String root, String namespace) {
+ if(!namespace.startsWith("/")) {
--- End diff --
missing space after if, and a second time a few lines downwards
---
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 #2249: 4166 zookeeper namespaces
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2249#discussion_r70883081
--- Diff: docs/setup/jobmanager_high_availability.md ---
@@ -74,11 +74,15 @@ In order to start an HA-cluster add the following configuration keys to `conf/fl
Each *addressX:port* refers to a ZooKeeper server, which is reachable by Flink at the given address and port.
-- **ZooKeeper root** (recommended): The *root ZooKeeper node*, under which all required coordination data is placed.
+- **ZooKeeper root** (recommended): The *root ZooKeeper node*, under which all cluster namespace nodes are placed.
- <pre>recovery.zookeeper.path.root: /flink # important: customize per cluster</pre>
+ <pre>recovery.zookeeper.path.root: /flink
- **Important**: if you are running multiple Flink HA clusters, you have to manually configure separate root nodes for each cluster.
+- **ZooKeeper namespace** (recommended): The *namespace ZooKeeper node*, under which all required coordination data for a cluster is placed.
+
+ <pre>recovery.zookeeper.path.namespace: /default_ns # important: customize per cluster</pre>
+
+ **Important**: if you are running multiple Flink HA clusters, you have to manually configure separate namespaces for each cluster. By default, Yarn cluster and Yarn session automatically generate namespaces based on Yarn application id. A manual configuration overrides this behaviour in Yarn. Specifying a namespace with the -z CLI option, in turn, overrides manual configuration.
--- End diff --
i believe you are missing a "the" before "Yarn cluster" and "Yarn application id"
---
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 #2249: [FLINK-4166] [CLI] Generate different namespaces f...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/2249
---
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 #2249: 4166 zookeeper namespaces
Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on the issue:
https://github.com/apache/flink/pull/2249
I addressed the valid comments and added some tests.
---
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 #2249: [FLINK-4166] [CLI] Generate different namespaces for Zook...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on the issue:
https://github.com/apache/flink/pull/2249
Thanks for the update. Could you rebase to the latest master?
---
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 #2249: 4166 zookeeper namespaces
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2249#discussion_r70883320
--- Diff: flink-yarn/src/main/java/org/apache/flink/yarn/AbstractYarnClusterDescriptor.java ---
@@ -545,6 +551,19 @@ protected YarnClusterClient deployInternal() throws Exception {
// Set-up ApplicationSubmissionContext for the application
ApplicationSubmissionContext appContext = yarnApplication.getApplicationSubmissionContext();
+ final ApplicationId appId = appContext.getApplicationId();
+
+ // ------------------ Add Zookeeper namespace to local flinkConfiguraton ------
+ String zkNamespace = getZookeeperNamespace();
+ // no user specified cli argument for namespace?
+ if(zkNamespace == null || zkNamespace.isEmpty()) {
--- End diff --
mising space after if
---
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 #2249: 4166 zookeeper namespaces
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2249#discussion_r70935614
--- Diff: flink-yarn/src/main/java/org/apache/flink/yarn/AbstractYarnClusterDescriptor.java ---
@@ -545,6 +551,19 @@ protected YarnClusterClient deployInternal() throws Exception {
// Set-up ApplicationSubmissionContext for the application
ApplicationSubmissionContext appContext = yarnApplication.getApplicationSubmissionContext();
+ final ApplicationId appId = appContext.getApplicationId();
+
+ // ------------------ Add Zookeeper namespace to local flinkConfiguraton ------
+ String zkNamespace = getZookeeperNamespace();
+ // no user specified cli argument for namespace?
+ if(zkNamespace == null || zkNamespace.isEmpty()) {
+ // namespace defined in config? else use applicationId as default.
+ zkNamespace = flinkConfiguration.getString(ConfigConstants.ZOOKEEPER_NAMESPACE_KEY, String.valueOf(appId));
+ setZookeeperNamespace(zkNamespace);
+ }
+
+ flinkConfiguration.setString(ConfigConstants.ZOOKEEPER_NAMESPACE_KEY, zkNamespace);
--- End diff --
ah you're right, if the key is not set but the condition is not true then we wouldn't set the property.
---
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 #2249: 4166 zookeeper namespaces
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2249#discussion_r70881905
--- Diff: flink-yarn/src/main/java/org/apache/flink/yarn/cli/FlinkYarnSessionCli.java ---
@@ -475,23 +484,23 @@ public YarnClusterClient retrieveCluster(
CommandLine cmdLine,
Configuration config) throws UnsupportedOperationException {
- // first check for an application id
- if (cmdLine.hasOption(APPLICATION_ID.getOpt())) {
- String applicationID = cmdLine.getOptionValue(APPLICATION_ID.getOpt());
+ // first check for an application id, then try to load from yarn properties
+ String applicationID = cmdLine.hasOption(APPLICATION_ID.getOpt()) ?
+ cmdLine.getOptionValue(APPLICATION_ID.getOpt())
+ : loadYarnPropertiesFile(cmdLine, config);
+
+ if(null != applicationID) {
--- End diff --
missing space after if
---
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 #2249: [FLINK-4166] [CLI] Generate different namespaces for Zook...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on the issue:
https://github.com/apache/flink/pull/2249
Thank you! Merging after tests pass.
---
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 #2249: 4166 zookeeper namespaces
Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on a diff in the pull request:
https://github.com/apache/flink/pull/2249#discussion_r70887209
--- Diff: flink-yarn/src/main/java/org/apache/flink/yarn/YarnApplicationMasterRunner.java ---
@@ -72,26 +68,36 @@
* This class is the executable entry point for the YARN application master.
* It starts actor system and the actors for {@link org.apache.flink.runtime.jobmanager.JobManager}
* and {@link YarnFlinkResourceManager}.
- *
+ * <p>
--- End diff --
sorry for that one, i accidentally hit the AF and noticed it too late.
---
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 #2249: 4166 zookeeper namespaces
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2249#discussion_r70935698
--- Diff: flink-yarn/src/main/java/org/apache/flink/yarn/AbstractYarnClusterDescriptor.java ---
@@ -545,6 +551,19 @@ protected YarnClusterClient deployInternal() throws Exception {
// Set-up ApplicationSubmissionContext for the application
ApplicationSubmissionContext appContext = yarnApplication.getApplicationSubmissionContext();
+ final ApplicationId appId = appContext.getApplicationId();
--- End diff --
ah, the diff confused me.
---
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 #2249: [FLINK-4166] [CLI] Generate different namespaces f...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/2249#discussion_r71178603
--- Diff: docs/setup/config.md ---
@@ -272,7 +272,9 @@ For example when running Flink on YARN on an environment with a restrictive fire
- `recovery.zookeeper.quorum`: Defines the ZooKeeper quorum URL which is used to connet to the ZooKeeper cluster when the 'zookeeper' recovery mode is selected
-- `recovery.zookeeper.path.root`: (Default '/flink') Defines the root dir under which the ZooKeeper recovery mode will create znodes.
+- `recovery.zookeeper.path.root`: (Default '/flink') Defines the root dir under which the ZooKeeper recovery mode will create namespace directories.
+
+- `recovery.zookeeper.path.namespace`: (Default '/default_ns' in standalone mode, or the <yarn-application-id> under Yarn) Defines the subdirectory under the root dir where the ZooKeeper recovery mode will create znodes. This allows to isolate multiple applications on the same ZooKeeper.
--- End diff --
It would be nice to have a default namespace for the standalone mode as well. How about generating a namespace from the `masters` files. Would hashing the contents of the file be a good fit? That should be consistent across all master nodes.
---
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 #2249: [FLINK-4166] [CLI] Generate different namespaces for Zook...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on the issue:
https://github.com/apache/flink/pull/2249
Travis cache seems to be corrupted. Have you run `mvn verify` 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 #2249: [FLINK-4166] [CLI] Generate different namespaces for Zook...
Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on the issue:
https://github.com/apache/flink/pull/2249
Thanks for the review, Max! I changed the default namespace string and rebased the the latest master.
---
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 #2249: [FLINK-4166] [CLI] Generate different namespaces f...
Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on a diff in the pull request:
https://github.com/apache/flink/pull/2249#discussion_r71178429
--- Diff: flink-core/src/main/java/org/apache/flink/configuration/ConfigConstants.java ---
@@ -969,6 +971,8 @@
public static final String DEFAULT_ZOOKEEPER_DIR_KEY = "/flink";
+ public static final String DEFAULT_ZOOKEEPER_NAMESPACE_KEY = "/default_ns";
--- End diff --
Sure, we can change that. I just thought that default might be a little generic.
---
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 #2249: [FLINK-4166] [CLI] Generate different namespaces f...
Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on a diff in the pull request:
https://github.com/apache/flink/pull/2249#discussion_r71309594
--- Diff: docs/setup/config.md ---
@@ -272,7 +272,9 @@ For example when running Flink on YARN on an environment with a restrictive fire
- `recovery.zookeeper.quorum`: Defines the ZooKeeper quorum URL which is used to connet to the ZooKeeper cluster when the 'zookeeper' recovery mode is selected
-- `recovery.zookeeper.path.root`: (Default '/flink') Defines the root dir under which the ZooKeeper recovery mode will create znodes.
+- `recovery.zookeeper.path.root`: (Default '/flink') Defines the root dir under which the ZooKeeper recovery mode will create namespace directories.
+
+- `recovery.zookeeper.path.namespace`: (Default '/default_ns' in standalone mode, or the <yarn-application-id> under Yarn) Defines the subdirectory under the root dir where the ZooKeeper recovery mode will create znodes. This allows to isolate multiple applications on the same ZooKeeper.
--- End diff --
I think the main problem with default namespaces in standalone mode is that there is no authority generating reliably unique identifier that we can use to label clusters (in contrast to e.g. application ids in Yarn). Also the contents of the masters file could be the same for two different clusters running on the same machines and collisions could happen. In particular, such collisions could be very rare and hence even more surprising to the user. Furthermore, I think that having multiple clusters in this way is exactly when you want to start Yarn or Mesos.
---
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 #2249: [FLINK-4166] [CLI] Generate different namespaces f...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/2249#discussion_r71176801
--- Diff: flink-core/src/main/java/org/apache/flink/configuration/ConfigConstants.java ---
@@ -969,6 +971,8 @@
public static final String DEFAULT_ZOOKEEPER_DIR_KEY = "/flink";
+ public static final String DEFAULT_ZOOKEEPER_NAMESPACE_KEY = "/default_ns";
--- End diff --
How about we change this to `default`? `ns` is pretty cryptic for most people anyways.
---
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 #2249: 4166 zookeeper namespaces
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2249#discussion_r70876542
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/util/ZooKeeperUtils.java ---
@@ -164,6 +169,7 @@ public static ZooKeeperLeaderElectionService createLeaderElectionService(
* @param client The {@link CuratorFramework} ZooKeeper client to use
* @param configuration {@link Configuration} object containing the configuration values
* @return {@link ZooKeeperLeaderElectionService} instance.
+ * @return {@link ZooKeeperLeaderElectionService} instance.
--- End diff --
duplicate `@return` entry
---
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 #2249: 4166 zookeeper namespaces
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2249#discussion_r70882042
--- Diff: flink-yarn/src/main/java/org/apache/flink/yarn/YarnApplicationMasterRunner.java ---
@@ -428,6 +429,12 @@ private static Configuration createConfiguration(String baseDirectory, Map<Strin
configuration.setString(property.getKey(), property.getValue());
}
+ // override zookeeper namespace with user cli argument (if provided)
+ String cliZKNamespace = ENV.get(YarnConfigKeys.ENV_ZOOKEEPER_NAMESPACE);
+ if(cliZKNamespace != null && !cliZKNamespace.isEmpty()) {
--- End diff --
missing space after if
---
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 #2249: [FLINK-4166] [CLI] Generate different namespaces for Zook...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on the issue:
https://github.com/apache/flink/pull/2249
The changes look good to me. Just had some comments about the default namespace name and an idea to generate a default namespace value in standalone mode.
---
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 #2249: 4166 zookeeper namespaces
Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on a diff in the pull request:
https://github.com/apache/flink/pull/2249#discussion_r70886483
--- Diff: flink-yarn/src/main/java/org/apache/flink/yarn/AbstractYarnClusterDescriptor.java ---
@@ -545,6 +551,19 @@ protected YarnClusterClient deployInternal() throws Exception {
// Set-up ApplicationSubmissionContext for the application
ApplicationSubmissionContext appContext = yarnApplication.getApplicationSubmissionContext();
+ final ApplicationId appId = appContext.getApplicationId();
--- End diff --
this line was only moved for my change. it is in fact used further down in the same method and cannot be moved inside the if.
---
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 #2249: 4166 zookeeper namespaces
Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on a diff in the pull request:
https://github.com/apache/flink/pull/2249#discussion_r70887016
--- Diff: flink-yarn/src/main/java/org/apache/flink/yarn/AbstractYarnClusterDescriptor.java ---
@@ -545,6 +551,19 @@ protected YarnClusterClient deployInternal() throws Exception {
// Set-up ApplicationSubmissionContext for the application
ApplicationSubmissionContext appContext = yarnApplication.getApplicationSubmissionContext();
+ final ApplicationId appId = appContext.getApplicationId();
+
+ // ------------------ Add Zookeeper namespace to local flinkConfiguraton ------
+ String zkNamespace = getZookeeperNamespace();
+ // no user specified cli argument for namespace?
+ if(zkNamespace == null || zkNamespace.isEmpty()) {
+ // namespace defined in config? else use applicationId as default.
+ zkNamespace = flinkConfiguration.getString(ConfigConstants.ZOOKEEPER_NAMESPACE_KEY, String.valueOf(appId));
+ setZookeeperNamespace(zkNamespace);
+ }
+
+ flinkConfiguration.setString(ConfigConstants.ZOOKEEPER_NAMESPACE_KEY, zkNamespace);
--- End diff --
simply moving this to the else block makes the code incorrect. i preferred the chance of overwriting a value with itself once in non performance critical code over bloating up the code.
---
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 #2249: [FLINK-4166] [CLI] Generate different namespaces f...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/2249#discussion_r71311352
--- Diff: docs/setup/config.md ---
@@ -272,7 +272,9 @@ For example when running Flink on YARN on an environment with a restrictive fire
- `recovery.zookeeper.quorum`: Defines the ZooKeeper quorum URL which is used to connet to the ZooKeeper cluster when the 'zookeeper' recovery mode is selected
-- `recovery.zookeeper.path.root`: (Default '/flink') Defines the root dir under which the ZooKeeper recovery mode will create znodes.
+- `recovery.zookeeper.path.root`: (Default '/flink') Defines the root dir under which the ZooKeeper recovery mode will create namespace directories.
+
+- `recovery.zookeeper.path.namespace`: (Default '/default_ns' in standalone mode, or the <yarn-application-id> under Yarn) Defines the subdirectory under the root dir where the ZooKeeper recovery mode will create znodes. This allows to isolate multiple applications on the same ZooKeeper.
--- End diff --
You're right, it would just be a heuristic for generating a unique namespace but it is not unique because multiple Flink instances might be running on the same host names (e.g. multiple processes, Docker containers). So +1 for not adding something that can cause hard to debug problems. We have covered the Yarn use case which makes it very convenient to setup a namespace.
---
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 #2249: 4166 zookeeper namespaces
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/2249
Please make sure to follow the naming conventions for pull requests, as described in the PR template. First entry under "General" : "The pull request references the related JIRA issue ("[FLINK-XXX] Jira title text")"
I can't speak for whether the implementation is correct, but the changes made don't appear to be covered by tests.
---
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.
---