[GitHub] flink pull request: FLINK-3327: ExecutionConfig to JobGraph.
Github user kl0u commented on the pull request: https://github.com/apache/flink/pull/1583#issuecomment-195605736 Thanks a lot! --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: FLINK-3327: ExecutionConfig to JobGraph.
Github user asfgit closed the pull request at: https://github.com/apache/flink/pull/1583 --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: FLINK-3327: ExecutionConfig to JobGraph.
Github user zentol commented on the pull request: https://github.com/apache/flink/pull/1583#issuecomment-195540338 merging this. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: FLINK-3327: ExecutionConfig to JobGraph.
Github user zentol commented on the pull request: https://github.com/apache/flink/pull/1583#issuecomment-195034317 are there any objections to merging this? let's get this one in. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: FLINK-3327: ExecutionConfig to JobGraph.
Github user kl0u commented on the pull request: https://github.com/apache/flink/pull/1583#issuecomment-187143786 Just rebased. Please review this PR. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: FLINK-3327: ExecutionConfig to JobGraph.
Github user kl0u commented on the pull request: https://github.com/apache/flink/pull/1583#issuecomment-185830388 Please Review this PR. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: FLINK-3327: ExecutionConfig to JobGraph.
Github user kl0u commented on the pull request: https://github.com/apache/flink/pull/1583#issuecomment-185734236 The problem is the CRLF vs LF characters in the committed versions of the files. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: FLINK-3327: ExecutionConfig to JobGraph.
Github user zentol commented on the pull request: https://github.com/apache/flink/pull/1583#issuecomment-185641724 Changes look good. Do you know what happened to the ExecutionStateProgressTest diff? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: FLINK-3327: ExecutionConfig to JobGraph.
Github user kl0u commented on the pull request: https://github.com/apache/flink/pull/1583#issuecomment-185637076 Please Review. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: FLINK-3327: ExecutionConfig to JobGraph.
Github user kl0u commented on the pull request: https://github.com/apache/flink/pull/1583#issuecomment-185210139 Hello! I just rebased to the new master. Please review this new PR. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: FLINK-3327: ExecutionConfig to JobGraph.
Github user kl0u commented on the pull request: https://github.com/apache/flink/pull/1583#issuecomment-185271802 Hello! I just rebased to the new master. Please review this new PR. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: FLINK-3327: ExecutionConfig to JobGraph.
Github user zentol commented on the pull request: https://github.com/apache/flink/pull/1583#issuecomment-184259383 Made a few remarks, but didn't find any issue that isn't a minor one. Good job! --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: FLINK-3327: ExecutionConfig to JobGraph.
Github user zentol commented on a diff in the pull request: https://github.com/apache/flink/pull/1583#discussion_r52912765 --- Diff: flink-yarn-tests/src/main/java/org/apache/flink/yarn/YarnTestBase.java --- @@ -112,7 +112,7 @@ * lib/ folder of the flink distribution. */ protected static File flinkLibFolder; - + --- End diff -- only change in this file is formatting --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: FLINK-3327: ExecutionConfig to JobGraph.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/1583#discussion_r52912585
--- Diff:
flink-streaming-java/src/test/java/org/apache/flink/streaming/api/graph/StreamingJobGraphGeneratorTest.java
---
@@ -75,16 +75,22 @@ public void testExecutionConfigSerialization() throws
IOException, ClassNotFound
config.disableSysoutLogging();
}
config.setParallelism(dop);
-
+
JobGraph jobGraph = compiler.createJobGraph("test");
-
+
+ final String exec_config_key = "runtime.config";
--- End diff --
variable doesn't follow naming conventions
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: FLINK-3327: ExecutionConfig to JobGraph.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/1583#discussion_r52912601
--- Diff:
flink-streaming-java/src/test/java/org/apache/flink/streaming/api/graph/StreamingJobGraphGeneratorTest.java
---
@@ -75,16 +75,22 @@ public void testExecutionConfigSerialization() throws
IOException, ClassNotFound
config.disableSysoutLogging();
}
config.setParallelism(dop);
-
+
JobGraph jobGraph = compiler.createJobGraph("test");
-
+
+ final String exec_config_key = "runtime.config";
+
+
InstantiationUtil.writeObjectToConfig(jobGraph.getExecutionConfig(),
+ jobGraph.getJobConfiguration(),
+ exec_config_key);
+
ExecutionConfig executionConfig =
InstantiationUtil.readObjectFromConfig(
jobGraph.getJobConfiguration(),
- ExecutionConfig.CONFIG_KEY,
+ exec_config_key,
Thread.currentThread().getContextClassLoader());
-
+
assertNotNull(executionConfig);
-
--- End diff --
this class contains a relatively high number of pure formatting changes.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: FLINK-3327: ExecutionConfig to JobGraph.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/1583#discussion_r52912014
--- Diff:
flink-runtime/src/main/scala/org/apache/flink/runtime/jobmanager/JobManager.scala
---
@@ -1051,7 +1052,8 @@ class JobManager(
}
catch {
case t: Throwable =>
- log.error(s"Failed to submit job $jobId ($jobName)", t)
+ val message = t.getMessage
+ log.error(s"Failed to submit job $jobId ($jobName): $message", t)
--- End diff --
does the exception's message not show up in the logs without this change?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: FLINK-3327: ExecutionConfig to JobGraph.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/1583#discussion_r52911198
--- Diff:
flink-runtime/src/main/java/org/apache/flink/runtime/jobgraph/JobGraph.java ---
@@ -225,22 +233,12 @@ public int getNumberOfExecutionRetries() {
* @return The delay of time in milliseconds the system will try to
re-execute failed tasks.
*/
public long getExecutionRetryDelay() {
- return executionRetryDelay;
- }
-
- /**
-* Sets the delay that failed tasks are re-executed. A value of zero
-* effectively disables fault tolerance. A value of {@code -1}
indicates that the system
-* default value (as defined in the configuration) should be used.
-*
-* @param executionRetryDelay The delay of time the system will wait to
re-execute failed tasks.
-*/
- public void setExecutionRetryDelay(long executionRetryDelay){
- if (executionRetryDelay < -1) {
+ long retryDelay = executionConfig.getExecutionRetryDelay();
+ if (retryDelay < -1) {
--- End diff --
same as above
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: FLINK-3327: ExecutionConfig to JobGraph.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/1583#discussion_r52911077
--- Diff:
flink-runtime/src/main/java/org/apache/flink/runtime/jobgraph/JobGraph.java ---
@@ -215,7 +218,12 @@ public void setNumberOfExecutionRetries(int
numberOfExecutionRetries) {
* @return The number of times the system will try to re-execute failed
tasks.
*/
public int getNumberOfExecutionRetries() {
- return numExecutionRetries;
+ int retries = executionConfig.getNumberOfExecutionRetries();
+ if (retries < -1) {
--- End diff --
this check shouldn't be necessary, since setNumberOfExecutionRetries
already checks for it.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: FLINK-3327: ExecutionConfig to JobGraph.
Github user zentol commented on the pull request: https://github.com/apache/flink/pull/1583#issuecomment-184252247 we could probably heavily reduce the diff by providing constructors with/without the executionconfig argument. (if none is given, we just instantiate a new one) --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: FLINK-3327: ExecutionConfig to JobGraph.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/1583#discussion_r52910257
--- Diff:
flink-clients/src/main/java/org/apache/flink/client/FlinkYarnSessionCli.java ---
@@ -393,7 +393,7 @@ public int run(String[] args) {
printUsage();
return 1;
}
-
+
--- End diff --
only change made in this file is formatting.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: FLINK-3327: ExecutionConfig to JobGraph.
Github user kl0u commented on the pull request: https://github.com/apache/flink/pull/1583#issuecomment-184236773 Hello! Just rebased to the new master. Please review. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: FLINK-3327: ExecutionConfig to JobGraph.
Github user kl0u commented on the pull request: https://github.com/apache/flink/pull/1583#issuecomment-182414285 Please review the new pull request. This pull request is the first step for the one about FLINK-2523. Thanks a lot! --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
