-
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-27903][runtime] Introduce and support HYBRID resultPartitionType #19927
Conversation
flink-core/src/main/java/org/apache/flink/api/common/BatchShuffleMode.java
Outdated
Show resolved
Hide resolved
flink-core/src/main/java/org/apache/flink/api/common/BatchShuffleMode.java
Outdated
Show resolved
Hide resolved
...runtime/src/main/java/org/apache/flink/runtime/io/network/partition/ResultPartitionType.java
Outdated
Show resolved
Hide resolved
...ng-java/src/main/java/org/apache/flink/streaming/api/transformations/StreamExchangeMode.java
Outdated
Show resolved
Hide resolved
...ming-java/src/main/java/org/apache/flink/streaming/api/graph/StreamingJobGraphGenerator.java
Show resolved
Hide resolved
...-java/src/test/java/org/apache/flink/streaming/api/graph/StreamingJobGraphGeneratorTest.java
Outdated
Show resolved
Hide resolved
.../java/org/apache/flink/runtime/scheduler/strategy/PipelinedRegionSchedulingStrategyTest.java
Outdated
Show resolved
Hide resolved
...ming-java/src/main/java/org/apache/flink/streaming/api/graph/StreamingJobGraphGenerator.java
Show resolved
Hide resolved
@flinkbot run azure |
@xintongsong This PR has been updated according to the comments, you can take a look when you have time. |
@flinkbot run azure |
* | ||
* <p>Intermediate data can be consumed directly from memory as much as possible. | ||
*/ | ||
HYBRID(true, false, ConsumingConstraint.CAN_BE_PIPELINED, ReleaseBy.SCHEDULER); |
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.
Is this type will be included in the isBlockingOrBlockingPersistentResultPartition
or isPipelinedOrPipelinedBoundedResultPartition
?
I found that after last change, these two new methods functionality is not very clear, It works like hard code check the Pipelined
and Blocking
type, Right?
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.
This will not be included in isBlockingOrBlockingPersistentResultPartition
or isPipelinedOrPipelinedBoundedResultPartition
.
The two methods here are exactly, as you said, checking for the specific types of the interface implementations. This is obviously not a good design, because you should not assume working with a specific implementation of the interface. However, this was not newly introduced. FLINK-27902 only explicitly separates use cases that rely on specific implementations from those that properly rely on the interfaces. Fixing of them probably requires more careful redesign in the problematic use cases, and we do not want to block this feature on that.
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 @xintongsong for your explanation. IMO, in the previous version, It mainly expose the isBlocking
and isPipelined
and isReconnectable
which I think is a characteristic and not bound to a specific implementation of the type. Please correct me if I'm wrong.
But I think we can redesign this part after this feature finished.
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.
Yes, the interfaces were intended to expose a characteristic rather than to be bounded to a specific implementation. The problem was on the caller side. Instead of relying on the intended characteristic, some callers were relying on assumptions such as "a result partition type that returns true
for isPipelined
must be PIPELINED
or PIPELINED_BOUNDED
".
E.g., in AdaptiveScheduler#assertPreconditions
, the intention here is to make sure only PIPELINED
and PIPELINED_BOUNDED
are involved. You may take a look at where isBlockingOrBlockingPersistentResultPartition
and isPipelinedOrPipelinedBoundedResultPartition
for more details.
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.
Get it, thanks
…o support testing for hybrid resultPartitionType.
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.
@reswqa, thanks for addressing the comments. LGTM.
I just made a bit minor doc changes, in the fixup commit. Please check it out.
@xintongsong Thanks for your review and good doc changes, Is the fixup commit will be rebased by me or when you merge? |
I'll handle the fixup commit during merging. |
…o support testing for hybrid resultPartitionType. This closes apache#19927
…o support testing for hybrid resultPartitionType. This closes apache#19927
…o support testing for hybrid resultPartitionType. This closes apache#19927
What is the purpose of the change
Introduce and support HYBRID resultPartitionType
Brief change log
Verifying this change
This change added unit tests.
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: yesDocumentation