You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by tison1 <gi...@git.apache.org> on 2018/07/17 13:31:52 UTC
[GitHub] flink pull request #6353: [FLINK-9875] Add concurrent creation of execution ...
GitHub user tison1 opened a pull request:
https://github.com/apache/flink/pull/6353
[FLINK-9875] Add concurrent creation of execution job vertex
## Add concurrent creation of execution job vertex
in some case like inputformat vertex, creation of execution job vertex is time
consuming, this pr add concurrent creation of execution job vertex to accelerate it.
## Brief change log
- `ExecutionGraph`
- add a method `createExecutionJobVertex` to concurrent creation of execution job vertex
- modify method `attachJobGraph` to acquire goodies from `createExecutionJobVertex`
## Verifying this change
Current tests confirm the correctness.
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): (no)
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: (no)
- The serializers: (no)
- The runtime per-record code paths (performance sensitive): (no)
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: (no)
- The S3 file system connector: (no)
## Documentation
- Does this pull request introduce a new feature? (no, it's internal)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/tison1/flink execution-vertex-init-improvement
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/6353.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 #6353
----
commit f219249ea99d25a41bb40aeb5a2d8c0d637f1cf7
Author: 陈梓立 <wa...@...>
Date: 2018-07-17T13:25:16Z
[FLINK-9875] Add concurrent creation of execution job vertex
----
---
[GitHub] flink issue #6353: [FLINK-9875] Add concurrent creation of execution job ver...
Posted by yanghua <gi...@git.apache.org>.
Github user yanghua commented on the issue:
https://github.com/apache/flink/pull/6353
you are right, this PR used to improve the performance when creating ejv, I did not make sure the exist test cases for `attachJobGraph ` covered the exception test. if not, I suggest add some exception test, because this PR changed the way of processing exception.
---
[GitHub] flink issue #6353: [FLINK-9875] Add concurrent creation of execution job ver...
Posted by tison1 <gi...@git.apache.org>.
Github user tison1 commented on the issue:
https://github.com/apache/flink/pull/6353
This pr cause `HITSITCase` unstable, I retrigger ci two times to get more info
going to fix it.
---
[GitHub] flink issue #6353: [FLINK-9875] Add concurrent creation of execution job ver...
Posted by tison1 <gi...@git.apache.org>.
Github user tison1 commented on the issue:
https://github.com/apache/flink/pull/6353
I think `ExecutionGraphConstructionTest` covers it.
---
[GitHub] flink issue #6353: [FLINK-9875][runtime] Add concurrent creation of executio...
Posted by TisonKun <gi...@git.apache.org>.
Github user TisonKun commented on the issue:
https://github.com/apache/flink/pull/6353
@StephanEwen you're right. there is something could be done to parallelize the creation of input split.
I wonder if the creation of InputSplit is always handled by InputFormat. There have been some ideas, and I will keep working on it.
Thanks for you advice!
---
[GitHub] flink issue #6353: [FLINK-9875] Add concurrent creation of execution job ver...
Posted by tison1 <gi...@git.apache.org>.
Github user tison1 commented on the issue:
https://github.com/apache/flink/pull/6353
Fix unstable case, the problem is code below, that may assign `constraints` to a long array and then to a short array, which cause out of index exception. to solve it we could init `constraints` in object construct.
https://github.com/apache/flink/blob/056486a1b81e9648a6d3dc795e7e2c6976f8388c/flink-runtime/src/main/java/org/apache/flink/runtime/jobmanager/scheduler/CoLocationGroup.java#L87-L91
---
[GitHub] flink issue #6353: [FLINK-9875][runtime] Add concurrent creation of executio...
Posted by yanghua <gi...@git.apache.org>.
Github user yanghua commented on the issue:
https://github.com/apache/flink/pull/6353
agree @StephanEwen , parallelize the core problem, this way we would not introduce potential concurrent problem of EG, EJV related logic.
---
[GitHub] flink issue #6353: [FLINK-9875] Add concurrent creation of execution job ver...
Posted by yanghua <gi...@git.apache.org>.
Github user yanghua commented on the issue:
https://github.com/apache/flink/pull/6353
@tison1 there are too many commits, you can use `git rebase -i [commit-id]` to squash them, then use `git push -f xxx xxx` to force update the PR.
---
[GitHub] flink issue #6353: [FLINK-9875] Add concurrent creation of execution job ver...
Posted by tison1 <gi...@git.apache.org>.
Github user tison1 commented on the issue:
https://github.com/apache/flink/pull/6353
the existing tests verify correctness. I will take a try to give out a benchmark report since this PR is more relevant to performance.
---
[GitHub] flink issue #6353: [FLINK-9875][runtime] Add concurrent creation of executio...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/6353
Maybe we can solve this simpler? Avoiding concurrency in the execution graph creation makes the code simpler and more robust - very desirable for an already fairly complex construct.
The issue here is the time it takes to create the splits, so we could see if we parallelize that, rather than parallelizing the job vertex creation.
I would think in the direction of having a Future that supplies the input splits and computing the future in the IOExecutor. That would parallelize the core problem and leave the executiongraph as it is.
---