-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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-34986][Runtime/State] Basic framework of async execution for state #24614
Conversation
Rebased and pushed again. |
@fredia @masteryhx would you please take a look? |
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.
@Zakelly Thanks for the PR, I left some comments first. And I haven't completely reviewed the test, some follow-up questions may be raised.
...k-runtime/src/main/java/org/apache/flink/runtime/asyncprocessing/ContextStateFutureImpl.java
Outdated
Show resolved
Hide resolved
...k-runtime/src/main/java/org/apache/flink/runtime/asyncprocessing/ContextStateFutureImpl.java
Show resolved
Hide resolved
flink-runtime/src/main/java/org/apache/flink/runtime/asyncprocessing/StateFutureFactory.java
Show resolved
Hide resolved
flink-runtime/src/main/java/org/apache/flink/runtime/asyncprocessing/StateRequest.java
Show resolved
Hide resolved
@fredia thank for your suggestions! |
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.
@Zakelly Thanks for the update, overall LGTM.
...ime/src/test/java/org/apache/flink/runtime/asyncprocessing/AsyncExecutionControllerTest.java
Show resolved
Hide resolved
...ntime/src/test/java/org/apache/flink/runtime/asyncprocessing/ContextStateFutureImplTest.java
Show resolved
Hide resolved
flink-runtime/src/main/java/org/apache/flink/runtime/asyncprocessing/StateRequest.java
Outdated
Show resolved
Hide resolved
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 the PR.
PTAL my comments.
flink-runtime/src/main/java/org/apache/flink/runtime/asyncprocessing/StateRequest.java
Outdated
Show resolved
Hide resolved
flink-runtime/src/main/java/org/apache/flink/runtime/asyncprocessing/StateRequest.java
Outdated
Show resolved
Hide resolved
flink-runtime/src/main/java/org/apache/flink/runtime/asyncprocessing/StateRequest.java
Outdated
Show resolved
Hide resolved
flink-runtime/src/main/java/org/apache/flink/runtime/asyncprocessing/StateExecutor.java
Show resolved
Hide resolved
flink-runtime/src/main/java/org/apache/flink/runtime/asyncprocessing/KeyAccountingUnit.java
Show resolved
Hide resolved
...runtime/src/main/java/org/apache/flink/runtime/asyncprocessing/AsyncExecutionController.java
Show resolved
Hide resolved
...ime/src/test/java/org/apache/flink/runtime/asyncprocessing/AsyncExecutionControllerTest.java
Outdated
Show resolved
Hide resolved
88f2e33
to
595a10c
Compare
@masteryhx Thanks for your comments! Applied some suggestions, PTAL thanks. |
...runtime/src/main/java/org/apache/flink/runtime/asyncprocessing/AsyncExecutionController.java
Show resolved
Hide resolved
...runtime/src/main/java/org/apache/flink/runtime/asyncprocessing/AsyncExecutionController.java
Show resolved
Hide resolved
...runtime/src/main/java/org/apache/flink/runtime/asyncprocessing/AsyncExecutionController.java
Show resolved
Hide resolved
42609e2
to
db14529
Compare
Rebased master. Extracting |
3bdab34
to
ab070bd
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 the PR. Left some comments as below.
flink-runtime/src/main/java/org/apache/flink/runtime/asyncprocessing/KeyAccountingUnit.java
Outdated
Show resolved
Hide resolved
* @return the state future. | ||
*/ | ||
public <IN, OUT> InternalStateFuture<OUT> handleRequest( | ||
@Nullable State state, StateRequestType type, @Nullable IN payload) { |
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.
Could you please remind me of the cases when a null
state would be passed to AEC?
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.
Added some description in javadoc. This is for the strict order of 'processElement' for same-key records.
flink-runtime/src/main/java/org/apache/flink/runtime/asyncprocessing/StateFutureFactory.java
Outdated
Show resolved
Hide resolved
...runtime/src/main/java/org/apache/flink/runtime/asyncprocessing/AsyncExecutionController.java
Outdated
Show resolved
Hide resolved
…xt and reference counting
…textStateFutureImpl
@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.
Thanks for the update! LGTM.
What is the purpose of the change
This PR ship the core part of FLIP-425, including the basic execution logic of AsyncExecutionController.
Brief change log
Verifying this change
org.apache.flink.runtime.asyncprocessing
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: noDocumentation