-
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-33341][state] Add support for rescaling from local keyed state #23591
[FLINK-33341][state] Add support for rescaling from local keyed state #23591
Conversation
3d2cd24
to
27090a0
Compare
27090a0
to
b8b8c48
Compare
b8b8c48
to
aa59dc1
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.
Nice changes @StefanRRichter!
I've only spotted one small issue with equals
, other things are mostly NITs.
Other than that LGTM.
public int hashCode() { | ||
int result = super.hashCode(); | ||
result = 31 * result + getMetaDataState().hashCode(); | ||
int result = directoryStateHandle.hashCode(); | ||
result = 31 * result + getKeyGroupRange().hashCode(); | ||
result = 31 * result + getMetaDataStateHandle().hashCode(); |
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.
- Doesn't
equals
need to be updated as well? I think it's currently doesn't matchhashCode
- nit: move closer to
equals
?
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.
Good catch, I think the hashCode/equals/toString in those classes should be consolidated. I'll create a separate commit to streamline them.
...k-runtime/src/main/java/org/apache/flink/runtime/state/IncrementalLocalKeyedStateHandle.java
Show resolved
Hide resolved
* | ||
* @param jobManagerState the state that the task got assigned from the job manager (this | ||
* state lives in remote storage). | ||
* @param alternativesByPriority local alternatives to the job manager starte, ordered by |
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.
typo "starte"
* | ||
* @param jobManagerState the state that the task got assigned from the job manager (this | ||
* state lives in remote storage). | ||
* @param alternativesByPriority local alternatives to the job manager starte, ordered by |
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.
typo "starte"
...e/src/test/java/org/apache/flink/runtime/checkpoint/PrioritizedOperatorSubtaskStateTest.java
Show resolved
Hide resolved
...e/src/test/java/org/apache/flink/runtime/checkpoint/PrioritizedOperatorSubtaskStateTest.java
Outdated
Show resolved
Hide resolved
… IncrementalKeyedStateHandle.
ca38a5e
to
31b79ef
Compare
…g for incremental state handle classes.
… to RockDBIncrementalRestoreOperation.
… to PrioritizedOperatorSubtaskState.
31b79ef
to
7013a86
Compare
What is the purpose of the change
This PR extends the use case for local (keyed) state from recovery to rescaling. Previously, Flink cannot make use of relevant local state in rescaling scenarios, even if such state would be available. For example, when scaling our we will often find already a superset of the state after scale out in the previously existing slots. And for scaling in, we should find a subset of the state in the existing slot. Utilizing such existing state can reduce the download from remote storage in rescaling scenarios.
Brief change log
AutoRescalingITCase
as a variant ofRescalingITCase
, that uses checkpoint & changing resource requirements to trigger a rescale that preservers local state (like an autoscaler would), instead of using savepoint & restart (losing local state).AbstractIncrementalStateHandle
as common superclass toIncrementalRemoteKeyedStateHandle
andIncrementalLocalKeyedStateHandle
. See next change for reason.RocksDBIncrementalRestoreOperation
in such way that we can transparently rescale fromIncrementalRemoteKeyedStateHandle
andIncrementalLocalKeyedStateHandle
(or a mix of both).PrioritizedOperatorSubtaskState
to support creating an alternative as a mix of local and remote (=assigned from job manager) state, instead of only alternatives that are either purely local ore purely remote.Verifying this change
This change added tests:
AutoRescalingITCase
PrioritizedOperatorSubtaskStateTest
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (no)Documentation