You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GJL <gi...@git.apache.org> on 2018/02/26 17:55:49 UTC
[GitHub] flink pull request #5584: [FLINK-8787][flip6] WIP
GitHub user GJL opened a pull request:
https://github.com/apache/flink/pull/5584
[FLINK-8787][flip6] WIP
WIP
## What is the purpose of the change
*(For example: This pull request makes task deployment go through the blob server, rather than through RPC. That way we avoid re-transferring them on each deployment (during recovery).)*
cc: @tillrohrmann
## Brief change log
*(for example:)*
- *The TaskInfo is stored in the blob store on job creation time as a persistent artifact*
- *Deployments RPC transmits only the blob storage reference*
- *TaskManagers retrieve the TaskInfo from the blob cache*
## Verifying this change
*(Please pick either of the following options)*
This change is a trivial rework / code cleanup without any test coverage.
*(or)*
This change is already covered by existing tests, such as *(please describe tests)*.
*(or)*
This change added tests and can be verified as follows:
*(example:)*
- *Added integration tests for end-to-end deployment with large payloads (100MB)*
- *Extended integration test for recovery after master (JobManager) failure*
- *Added test that validates that TaskInfo is transferred only once across recoveries*
- *Manually verified the change by running a 4 node cluser with 2 JobManagers and 4 TaskManagers, a stateful streaming program, and killing one JobManager and two TaskManagers during the execution, verifying that recovery happens correctly.*
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): (yes / no)
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: (yes / no)
- The serializers: (yes / no / don't know)
- The runtime per-record code paths (performance sensitive): (yes / no / don't know)
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: (yes / no / don't know)
- The S3 file system connector: (yes / no / don't know)
## Documentation
- Does this pull request introduce a new feature? (yes / no)
- If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/GJL/flink FLINK-8787
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/5584.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 #5584
----
commit 0cde09add17106f09e1f44b2a73400ea14a9eb21
Author: gyao <ga...@...>
Date: 2018-02-26T17:52:18Z
[hotfix] Add requireNonNull validation to Configuration copy constructor
commit 8f826c673bafd8228bb25da20e7dd40384fae971
Author: gyao <ga...@...>
Date: 2018-02-26T17:54:34Z
[FLINK-8787][flip6] Ensure that zk namespace configuration reaches RestClusterClient
----
---
[GitHub] flink issue #5584: [FLINK-8787][flip6] WIP Deploying FLIP-6 YARN session wit...
Posted by GJL <gi...@git.apache.org>.
Github user GJL commented on the issue:
https://github.com/apache/flink/pull/5584
@tillrohrmann Because I am creating a defensive copy of the configuration. Changing the config outside of the AbstractYarnClusterDescriptor won't have side effects. I will rewrite the test.
---
[GitHub] flink pull request #5584: [FLINK-8787][flip6] Deploying FLIP-6 YARN session ...
Posted by GJL <gi...@git.apache.org>.
Github user GJL closed the pull request at:
https://github.com/apache/flink/pull/5584
---
[GitHub] flink pull request #5584: [FLINK-8787][flip6] WIP Deploying FLIP-6 YARN sess...
Posted by GJL <gi...@git.apache.org>.
Github user GJL commented on a diff in the pull request:
https://github.com/apache/flink/pull/5584#discussion_r170882807
--- Diff: flink-yarn/src/main/java/org/apache/flink/yarn/AbstractYarnClusterDescriptor.java ---
@@ -186,7 +187,7 @@ public YarnClient getYarnClient() {
protected abstract String getYarnJobClusterEntrypoint();
public Configuration getFlinkConfiguration() {
- return flinkConfiguration;
+ return new UnmodifiableConfiguration(flinkConfiguration);
--- End diff --
fixed
---
[GitHub] flink pull request #5584: [FLINK-8787][flip6] Deploying FLIP-6 YARN session ...
Posted by GJL <gi...@git.apache.org>.
Github user GJL closed the pull request at:
https://github.com/apache/flink/pull/5584
---
[GitHub] flink pull request #5584: [FLINK-8787][flip6] Deploying FLIP-6 YARN session ...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/5584#discussion_r170922634
--- Diff: flink-yarn/src/main/java/org/apache/flink/yarn/AbstractYarnClusterDescriptor.java ---
@@ -479,36 +479,60 @@ public void terminateCluster(ApplicationId applicationId) throws FlinkException
ClusterEntrypoint.ExecutionMode.DETACHED
: ClusterEntrypoint.ExecutionMode.NORMAL;
- flinkConfiguration.setString(ClusterEntrypoint.EXECUTION_MODE, executionMode.toString());
+ effectiveConfiguration.setString(ClusterEntrypoint.EXECUTION_MODE, executionMode.toString());
+
+ final ApplicationId appId = yarnApplication.getApplicationSubmissionContext().getApplicationId();
- ApplicationReport report = startAppMaster(
- new Configuration(flinkConfiguration),
+ // --------- Add Zookeeper namespace to effectiveConfiguration ---------
+ 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(HighAvailabilityOptions.HA_CLUSTER_ID, appId.toString());
+ setZookeeperNamespace(zkNamespace);
+ }
+ effectiveConfiguration.setString(HighAvailabilityOptions.HA_CLUSTER_ID, zkNamespace);
--- End diff --
This would then mean that we only write the `appId` as the `HA_CLUSTER_ID` if there is no value set.
---
[GitHub] flink pull request #5584: [FLINK-8787][flip6] Deploying FLIP-6 YARN session ...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/5584#discussion_r170921732
--- Diff: flink-yarn/src/main/java/org/apache/flink/yarn/AbstractYarnClusterDescriptor.java ---
@@ -479,36 +479,60 @@ public void terminateCluster(ApplicationId applicationId) throws FlinkException
ClusterEntrypoint.ExecutionMode.DETACHED
: ClusterEntrypoint.ExecutionMode.NORMAL;
- flinkConfiguration.setString(ClusterEntrypoint.EXECUTION_MODE, executionMode.toString());
+ effectiveConfiguration.setString(ClusterEntrypoint.EXECUTION_MODE, executionMode.toString());
+
+ final ApplicationId appId = yarnApplication.getApplicationSubmissionContext().getApplicationId();
- ApplicationReport report = startAppMaster(
- new Configuration(flinkConfiguration),
+ // --------- Add Zookeeper namespace to effectiveConfiguration ---------
+ 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(HighAvailabilityOptions.HA_CLUSTER_ID, appId.toString());
+ setZookeeperNamespace(zkNamespace);
+ }
+ effectiveConfiguration.setString(HighAvailabilityOptions.HA_CLUSTER_ID, zkNamespace);
+
+ effectiveConfiguration.setInteger(
+ ConfigConstants.TASK_MANAGER_NUM_TASK_SLOTS,
+ clusterSpecification.getSlotsPerTaskManager());
+
+ // write out configuration file so that it can be uploaded
+ File tmpConfigurationFile = File.createTempFile(appId + "-flink-conf.yaml", null);
+ tmpConfigurationFile.deleteOnExit();
+ BootstrapTools.writeConfiguration(effectiveConfiguration, tmpConfigurationFile);
--- End diff --
Why do we pull the writing of the configuration out of `startAppMaster`? This belongs somewhat to the `startAppMaster` method, because then it could also be cleaned up there after starting the application master.
---
[GitHub] flink issue #5584: [FLINK-8787][flip6] WIP Deploying FLIP-6 YARN session wit...
Posted by GJL <gi...@git.apache.org>.
Github user GJL commented on the issue:
https://github.com/apache/flink/pull/5584
test failures due to
```
// logback + log4j, with/out krb5, different JVM opts
// IMPORTANT: Be aware that we are using side effects here to modify the created YarnClusterDescriptor,
// because we have a reference to the ClusterDescriptor's configuration which we modify continuously
```
---
[GitHub] flink pull request #5584: [FLINK-8787][flip6] Deploying FLIP-6 YARN session ...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/5584#discussion_r170922371
--- Diff: flink-yarn/src/main/java/org/apache/flink/yarn/AbstractYarnClusterDescriptor.java ---
@@ -479,36 +479,60 @@ public void terminateCluster(ApplicationId applicationId) throws FlinkException
ClusterEntrypoint.ExecutionMode.DETACHED
: ClusterEntrypoint.ExecutionMode.NORMAL;
- flinkConfiguration.setString(ClusterEntrypoint.EXECUTION_MODE, executionMode.toString());
+ effectiveConfiguration.setString(ClusterEntrypoint.EXECUTION_MODE, executionMode.toString());
+
+ final ApplicationId appId = yarnApplication.getApplicationSubmissionContext().getApplicationId();
- ApplicationReport report = startAppMaster(
- new Configuration(flinkConfiguration),
+ // --------- Add Zookeeper namespace to effectiveConfiguration ---------
+ 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(HighAvailabilityOptions.HA_CLUSTER_ID, appId.toString());
+ setZookeeperNamespace(zkNamespace);
--- End diff --
I think we could remove the whole `setZookeeperNamespace` and `getZookeeperNamespace` methods + field and simply rely on the fact that the input `Configuration` has the correct field set if there was a respective option set.
---
[GitHub] flink issue #5584: [FLINK-8787][flip6] Deploying FLIP-6 YARN session with HA...
Posted by GJL <gi...@git.apache.org>.
Github user GJL commented on the issue:
https://github.com/apache/flink/pull/5584
There are more problems.
---
[GitHub] flink pull request #5584: [FLINK-8787][flip6] WIP
Posted by GJL <gi...@git.apache.org>.
Github user GJL commented on a diff in the pull request:
https://github.com/apache/flink/pull/5584#discussion_r170679952
--- Diff: flink-yarn/src/main/java/org/apache/flink/yarn/AbstractYarnClusterDescriptor.java ---
@@ -186,7 +187,7 @@ public YarnClient getYarnClient() {
protected abstract String getYarnJobClusterEntrypoint();
public Configuration getFlinkConfiguration() {
- return flinkConfiguration;
+ return new UnmodifiableConfiguration(flinkConfiguration);
--- End diff --
not needed, `flinkConfiguration` is unmodifiable already
---
[GitHub] flink issue #5584: [FLINK-8787][flip6] WIP Deploying FLIP-6 YARN session wit...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on the issue:
https://github.com/apache/flink/pull/5584
Why are the tests failing due to this @GJL?
---
[GitHub] flink pull request #5584: [FLINK-8787][flip6] Deploying FLIP-6 YARN session ...
Posted by GJL <gi...@git.apache.org>.
GitHub user GJL reopened a pull request:
https://github.com/apache/flink/pull/5584
[FLINK-8787][flip6] Deploying FLIP-6 YARN session with HA fails
## What is the purpose of the change
*Deploying FLIP-6 YARN session with HA fails because the ZooKeeper namespace is not set correctly. The namespace is `/flink/default` but it should include the YARN application id.*
cc: @tillrohrmann
## Brief change log
- *Use defensive copies of `Configuration` objects in `AbstractYarnClusterDescriptor`. Never modify existing configuration.*
- *Ensure that zk namespace configuration reaches RestClusterClient*
## Verifying this change
This change added tests and can be verified as follows:
- *Manually deployed Flink on YARN with HA enabled.*
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): (yes / **no**)
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: (yes / **no**)
- The serializers: (yes / **no** / don't know)
- The runtime per-record code paths (performance sensitive): (yes / **no** / don't know)
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: (**yes** / no / don't know)
- The S3 file system connector: (yes / **no** / don't know)
## Documentation
- Does this pull request introduce a new feature? (yes / **no**)
- If yes, how is the feature documented? (**not applicable** / docs / JavaDocs / not documented)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/GJL/flink FLINK-8787
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/5584.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 #5584
----
commit 0cde09add17106f09e1f44b2a73400ea14a9eb21
Author: gyao <ga...@...>
Date: 2018-02-26T17:52:18Z
[hotfix] Add requireNonNull validation to Configuration copy constructor
commit 17e9ba4142fa003528ee5f6ecc8d06f70bcb35c3
Author: gyao <ga...@...>
Date: 2018-02-26T17:54:34Z
[FLINK-8787][flip6] Ensure that zk namespace configuration reaches RestClusterClient
commit 17d346908d6def6e36b99f9c536ebd731665e991
Author: gyao <ga...@...>
Date: 2018-02-27T10:45:03Z
fixup! [FLINK-8787][flip6] Ensure that zk namespace configuration reaches RestClusterClient
----
---
[GitHub] flink issue #5584: [FLINK-8787][flip6] WIP Deploying FLIP-6 YARN session wit...
Posted by GJL <gi...@git.apache.org>.
Github user GJL commented on the issue:
https://github.com/apache/flink/pull/5584
Yes, can do.
---