-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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-18990][task] Read channel state sequentially #13351
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 3c79716 (Tue Sep 08 09:04:30 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:
|
a219f94
to
8e0da15
Compare
8e0da15
to
df935f3
Compare
df935f3
to
4a598b9
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.
Very intriguing solution. I left a couple of comments but nothing deep. While reviewing my main issue was to understand the relationship between Sequential and old ChannelStateReader (become only clear after last commit). Last commits looks a bit incomplete.
Note that I still struggle a bit on the concepts of StateHandle and delegate. I dropped a question at the main commit.
flink-runtime/src/test/java/org/apache/flink/runtime/operators/testutils/DummyEnvironment.java
Show resolved
Hide resolved
flink-streaming-java/src/main/java/org/apache/flink/streaming/runtime/tasks/StreamTask.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/apache/flink/runtime/checkpoint/channel/SequentialChannelStateReader.java
Show resolved
Hide resolved
.../main/java/org/apache/flink/runtime/checkpoint/channel/SequentialChannelStateReaderImpl.java
Outdated
Show resolved
Hide resolved
.../main/java/org/apache/flink/runtime/checkpoint/channel/SequentialChannelStateReaderImpl.java
Outdated
Show resolved
Hide resolved
...ntime/src/main/java/org/apache/flink/runtime/io/network/api/reader/AbstractRecordReader.java
Show resolved
Hide resolved
.../main/java/org/apache/flink/runtime/io/network/partition/consumer/RecoveredInputChannel.java
Outdated
Show resolved
Hide resolved
...rc/test/java/org/apache/flink/runtime/checkpoint/channel/ChannelStateSerializerImplTest.java
Show resolved
Hide resolved
...rc/test/java/org/apache/flink/runtime/checkpoint/channel/ChannelStateSerializerImplTest.java
Show resolved
Hide resolved
flink-runtime/src/test/java/org/apache/flink/runtime/state/ChannelPersistenceITCase.java
Show resolved
Hide resolved
d5de65d
to
ce1ddf0
Compare
Thanks for reviewing @AHeise and @pnowojski |
...va/org/apache/flink/streaming/runtime/tasks/StreamTaskMultipleInputSelectiveReadingTest.java
Outdated
Show resolved
Hide resolved
ce1ddf0
to
f19eaa5
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.
Addressed most of my comments, a few open discussions.
I'm thinking that the last 3 commits (remove tests, methods, class of channel state reader) should be squashed.
flink-streaming-java/src/test/java/org/apache/flink/streaming/util/StreamTaskUtil.java
Show resolved
Hide resolved
...e/src/test/java/org/apache/flink/runtime/checkpoint/channel/ChannelStateChunkReaderTest.java
Outdated
Show resolved
Hide resolved
...t/java/org/apache/flink/runtime/checkpoint/channel/SequentialChannelStateReaderImplTest.java
Show resolved
Hide resolved
f19eaa5
to
78668b9
Compare
@flinkbot run azure |
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.
LGTM % squash last 3 commits. I'm still waiting for @pnowojski to look at the changed test.
...va/org/apache/flink/streaming/runtime/tasks/StreamTaskMultipleInputSelectiveReadingTest.java
Outdated
Show resolved
Hide resolved
78668b9
to
e8c0336
Compare
Thanks for reviewing @AHeise and @pnowojski . |
...va/org/apache/flink/streaming/runtime/tasks/StreamTaskMultipleInputSelectiveReadingTest.java
Outdated
Show resolved
Hide resolved
e8c0336
to
3e38dfe
Compare
Build failure unrelated: FLINK-19427 |
ec6d68e
to
3041c48
Compare
This removes casts to RemoteInputChannel and RecoveredInputChannel
Non-sequential state reader and the associated code will be removed in subsequent commits.
3041c48
to
59ca6fe
Compare
I rebased the PR and resolved conflicts with FLINK-19027 by moving the assignment of exclusive buffers for I also replaced @pnowojski, @AHeise please take a look. |
.../src/main/resources/archetype-resources/${project.build.directory}/classes/log4j2.properties
Outdated
Show resolved
Hide resolved
Motivation: reduce the required test coverage (tests will be removed in a later commit).
…ter state was fully consumed Motivation: 1. Guarantee state is not lost 2. Reduce the required test coverage (tests will be removed in the next commit)
…elated code - ChannelStateReader and supporting classes - methods for channel state recovery in inputChannels, subpartitions, etc. - tests for reading channel state non-sequentially
59ca6fe
to
c0b8f40
Compare
What is the purpose of the change
Read channel state sequentially to reduce the number of seeks.
Verifying this change
SequentialChannelStateReaderImplTest
andChannelStateChunkReaderTest
for the new code.RecoveredInputChannelTest
to check that conversion is not permitted untill fully recovered and state consumedStreamTask
is read unconditionally; 2) recovered channel can't be converted without being fully consumed anymoreDoes this pull request potentially affect one of the following parts:
@Public(Evolving)
: (yes / no)Documentation