-
Notifications
You must be signed in to change notification settings - Fork 13k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FLINK-17016][runtime] Integrate pipelined region scheduling #13284
[FLINK-17016][runtime] Integrate pipelined region scheduling #13284
Conversation
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Automated ChecksLast check on commit 6e68f6b (Mon Aug 31 09:35:10 UTC 2020) Warnings:
Mention the bot in a comment to re-run the automated checks. Review Progress
Please see the Pull Request Review Guide for a full explanation of the review process. The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commandsThe @flinkbot bot supports the following commands:
|
The failed e2e test "Elasticsearch (v6.3.1) sink" is a known issue FLINK-19093 which is unrelated to this change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for opening the PR @zhuzhurk
I think it is good, I left some smaller comments
flink-runtime/src/main/java/org/apache/flink/runtime/jobgraph/JobGraph.java
Outdated
Show resolved
Hide resolved
...-runtime/src/main/java/org/apache/flink/runtime/scheduler/ExecutionSlotAllocatorFactory.java
Outdated
Show resolved
Hide resolved
...-runtime/src/main/java/org/apache/flink/runtime/scheduler/ExecutionSlotAllocatorFactory.java
Outdated
Show resolved
Hide resolved
flink-tests/src/test/java/org/apache/flink/test/scheduling/PipelinedRegionSchedulingITCase.java
Show resolved
Hide resolved
flink-tests/src/test/java/org/apache/flink/test/scheduling/PipelinedRegionSchedulingITCase.java
Outdated
Show resolved
Hide resolved
21bf743
to
e5a5f7d
Compare
9ab00d3
to
e95e0ab
Compare
flink-core/src/main/java/org/apache/flink/configuration/JobManagerOptions.java
Show resolved
Hide resolved
flink-runtime/src/main/java/org/apache/flink/runtime/scheduler/DefaultSchedulerFactory.java
Outdated
Show resolved
Hide resolved
flink-tests/src/test/java/org/apache/flink/test/scheduling/PipelinedRegionSchedulingITCase.java
Show resolved
Hide resolved
flink-runtime/src/main/java/org/apache/flink/runtime/scheduler/DefaultScheduler.java
Outdated
Show resolved
Hide resolved
flink-runtime/src/test/java/org/apache/flink/runtime/minicluster/MiniClusterITCase.java
Outdated
Show resolved
Hide resolved
e95e0ab
to
acc8ba0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing comments @zhuzhurk
The PR looks good to me, only minor comments. There are test failures though
@@ -35,7 +35,7 @@ | |||
import static org.junit.Assert.assertThat; | |||
|
|||
/** | |||
* Tests for {@link DefaultSchedulerFactory}. | |||
* Tests for {@link DefaultSchedulerComponents}. | |||
*/ | |||
public class DefaultSchedulerFactoryTest extends TestLogger { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public class DefaultSchedulerFactoryTest extends TestLogger { | |
public class DefaultSchedulerComponentsFactoryTest extends TestLogger { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
@@ -35,7 +35,7 @@ | |||
import static org.junit.Assert.assertThat; | |||
|
|||
/** | |||
* Tests for {@link DefaultSchedulerFactory}. | |||
* Tests for {@link DefaultSchedulerComponents}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Tests for {@link DefaultSchedulerComponents}. | |
* Tests for {@link DefaultSchedulerComponents#createSchedulerComponents}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
import java.util.function.Consumer; | ||
|
||
/** | ||
* Components to create a {@link DefaultScheduler}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Components to create a {@link DefaultScheduler}. | |
* Components to create a {@link DefaultScheduler} which depend on the configured {@link JobManagerOptions#SCHEDULING_STRATEGY}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
95fb9ba
to
b50f961
Compare
The commits to "enabled pipelined region scheduling by default" are dropped. |
b50f961
to
e62b168
Compare
…tegy It can be enabled via config option "jobmanager.scheduler.scheduling-strategy=region"
e62b168
to
a687e9a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for address my comments @zhuzhurk LGTM
Thanks for reviewing! @azagrebin |
What is the purpose of the change
This PR is to integrate pipelined region scheduling with DefaultScheduler.
A new config option "jobmanager.scheduler.scheduling-strategy" is introduced to control whether to use the new "region" scheduling or to use the "legacy" eager/lazy-from-sources scheduling.
This change is based on #13181 and #13321
Brief change log
Verifying this change
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (yes / no)Documentation