Skip to content

Conversation

kl0u
Copy link
Contributor

@kl0u kl0u commented Feb 3, 2016

This makes the ExecutionConfig available to the Task. In a nutshell, the ExecutionConfig is attached to the JobGraph which is sent to the JobManager. The JobManager passes it to the ExecutionGraph, and, later on, to the TaskDeploymentDescriptor, and the TaskManager puts it into the Environment, which is visible to the AbstractInvocable.

@kl0u kl0u force-pushed the config_to_jobgraph branch 6 times, most recently from 7fe608d to fdf74f0 Compare February 9, 2016 09:56
@StephanEwen
Copy link
Contributor

Looks good at a first glance. I'll try to go through it and see if I can merge it...

@kl0u
Copy link
Contributor Author

kl0u commented Feb 9, 2016

Thanks a lot @StephanEwen

@kl0u kl0u force-pushed the config_to_jobgraph branch 4 times, most recently from 3fd32bb to f2594da Compare February 9, 2016 23:37
@kl0u kl0u changed the title ExecutionConfig to JobGraph. FLINK-3327: ExecutionConfig to JobGraph. Feb 10, 2016
@kl0u kl0u force-pushed the config_to_jobgraph branch 2 times, most recently from 3105029 to 84299ba Compare February 10, 2016 14:48
@kl0u
Copy link
Contributor Author

kl0u commented Feb 10, 2016

Please review the new pull request.
This pull request is the first step for the one about FLINK-2523.

Thanks a lot!

@kl0u kl0u force-pushed the config_to_jobgraph branch from 84299ba to aa6ca2b Compare February 15, 2016 13:14
@kl0u
Copy link
Contributor Author

kl0u commented Feb 15, 2016

Hello! Just rebased to the new master. Please review.

return 1;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only change made in this file is formatting.

@zentol
Copy link
Contributor

zentol commented Feb 15, 2016

we could probably heavily reduce the diff by providing JobGraph constructors with/without the executionconfig argument. (if none is given, we just instantiate a new one)

public int getNumberOfExecutionRetries() {
return numExecutionRetries;
int retries = executionConfig.getNumberOfExecutionRetries();
if (retries < -1) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this check shouldn't be necessary, since setNumberOfExecutionRetries already checks for it.

@zentol
Copy link
Contributor

zentol commented Feb 15, 2016

Made a few remarks, but didn't find any issue that isn't a minor one. Good job!

@kl0u kl0u force-pushed the config_to_jobgraph branch 3 times, most recently from 6b89c89 to df3a84f Compare February 17, 2016 15:35
@kl0u kl0u force-pushed the config_to_jobgraph branch 2 times, most recently from 948e978 to 2495fea Compare February 17, 2016 16:16
@kl0u
Copy link
Contributor Author

kl0u commented Feb 18, 2016

Please Review.

@zentol
Copy link
Contributor

zentol commented Feb 18, 2016

Changes look good. Do you know what happened to the ExecutionStateProgressTest diff?

@kl0u kl0u force-pushed the config_to_jobgraph branch 2 times, most recently from 7c690c5 to de135e2 Compare February 18, 2016 13:44
@kl0u
Copy link
Contributor Author

kl0u commented Feb 18, 2016

The problem is the CRLF vs LF characters in the committed versions of the files.

@kl0u
Copy link
Contributor Author

kl0u commented Feb 18, 2016

Please Review this PR.

@kl0u kl0u force-pushed the config_to_jobgraph branch 2 times, most recently from 519432b to ea35baa Compare February 22, 2016 11:03
@kl0u
Copy link
Contributor Author

kl0u commented Feb 22, 2016

Just rebased. Please review this PR.

@kl0u kl0u force-pushed the config_to_jobgraph branch 2 times, most recently from 8f7d9fa to 7ced60c Compare March 5, 2016 14:34
@kl0u kl0u force-pushed the config_to_jobgraph branch from 7ced60c to 8722e3f Compare March 7, 2016 09:42
@kl0u kl0u force-pushed the config_to_jobgraph branch from 8722e3f to 59bcbfe Compare March 7, 2016 12:34
@kl0u
Copy link
Contributor Author

kl0u commented Mar 7, 2016

@StephanEwen , @zentol I just rebased to the latest master.
It passes all tests on my Travis (https://travis-ci.org/kl0u/flink/builds/114228524).

Please review because the PR touches a lot of classes.

@zentol
Copy link
Contributor

zentol commented Mar 7, 2016

looks like something went wrong while rebasing, a commit by aljoscha suddenly popped up.

but hey, that can be fixed while merging, +1 from me.

@kl0u
Copy link
Contributor Author

kl0u commented Mar 7, 2016

Thanks a lot @zentol

@zentol
Copy link
Contributor

zentol commented Mar 10, 2016

are there any objections to merging this? let's get this one in.

@zentol
Copy link
Contributor

zentol commented Mar 11, 2016

merging this.

@asfgit asfgit closed this in 0f8d76c Mar 11, 2016
@kl0u
Copy link
Contributor Author

kl0u commented Mar 11, 2016

Thanks a lot!

subhankarb pushed a commit to subhankarb/flink that referenced this pull request Mar 17, 2016
@kl0u kl0u deleted the config_to_jobgraph branch June 9, 2016 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants