-
Notifications
You must be signed in to change notification settings - Fork 2.5k
fix(flink): fail fast when a bounded HoodieSource resumes a checkpoin… #19376
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,6 +21,7 @@ | |
| import org.apache.hudi.common.util.Option; | ||
| import org.apache.hudi.source.split.HoodieSourceSplitState; | ||
|
|
||
| import lombok.AllArgsConstructor; | ||
| import lombok.Value; | ||
|
|
||
| import java.io.Serializable; | ||
|
|
@@ -30,9 +31,35 @@ | |
| * State of Hoodie split enumerator. Mainly include the states of pending splits of split provider. | ||
| */ | ||
| @Value | ||
| @AllArgsConstructor | ||
| public class HoodieSplitEnumeratorState implements Serializable { | ||
|
|
||
| Collection<HoodieSourceSplitState> pendingSplitStates; | ||
| Option<String> lastEnumeratedInstant; | ||
| Option<String> lastEnumeratedInstantOffset; | ||
| /** | ||
| * The {@code read.start-commit} / {@code read.end-commit} bounds configured when this checkpoint | ||
| * was taken, recorded only for bounded reads. Both hold {@code Option.of("")} when the option was | ||
| * not configured, so that "recorded but unset" stays distinguishable from "not recorded at all"; | ||
| * both are {@link Option#empty()} for streaming reads and for checkpoints written by serializer | ||
| * VERSION 1, which predates this field. | ||
| * | ||
| * <p>A bounded read's split set is frozen at enumeration time and is NOT re-derived on restore, so | ||
| * {@code HoodieSource} compares these against the configured bounds and fails fast when they | ||
| * differ. See {@code HoodieSource#checkBoundedCommitRangeUnchanged}. | ||
| */ | ||
| Option<String> readStartCommit; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. since we already have
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For bounded reads there's nothing in those two to infer from — they're Option.empty() on every bounded checkpoint, and were before this PR too (HoodieStaticSplitEnumerator inherited AbstractHoodieSplitEnumerator:129; HoodieContinuousSplitEnumerator:90 is the only writer). So recovery can't reconstruct the configured range from state alone. On cost: it's two optional strings per checkpoint, written only by the bounded enumerator, alongside the pending-split collection. I also tried deriving from the splits' InstantRange — no new state at all — but IncrementalQueryAnalyzer:443-455 returns either no range or a null start bound when reading from earliest, and only incremental non-CDC splits carry one. |
||
| Option<String> readEndCommit; | ||
|
|
||
| /** | ||
| * Backward-compatible constructor for callers that do not record a commit range: the streaming | ||
| * enumerator and pre-existing tests. Leaves both bounds {@link Option#empty()}. | ||
| */ | ||
| public HoodieSplitEnumeratorState( | ||
| Collection<HoodieSourceSplitState> pendingSplitStates, | ||
| Option<String> lastEnumeratedInstant, | ||
| Option<String> lastEnumeratedInstantOffset) { | ||
| this(pendingSplitStates, lastEnumeratedInstant, lastEnumeratedInstantOffset, | ||
| Option.empty(), Option.empty()); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,19 +18,66 @@ | |
|
|
||
| package org.apache.hudi.source.enumerator; | ||
|
|
||
| import org.apache.hudi.common.util.Option; | ||
| import org.apache.hudi.exception.HoodieException; | ||
| import org.apache.hudi.source.split.HoodieSourceSplit; | ||
| import org.apache.hudi.source.split.HoodieSplitProvider; | ||
|
|
||
| import lombok.extern.slf4j.Slf4j; | ||
| import org.apache.flink.api.connector.source.SplitEnumeratorContext; | ||
| import org.apache.flink.runtime.execution.SuppressRestartsException; | ||
|
|
||
| /** | ||
| * Static Hoodie split enumerator that only handles with a bounded number of hudi commits. | ||
| */ | ||
| @Slf4j | ||
| public class HoodieStaticSplitEnumerator extends AbstractHoodieSplitEnumerator { | ||
|
|
||
| // The read.start-commit / read.end-commit bounds this bounded read was enumerated with, persisted | ||
| // in the enumerator checkpoint so a later restore can detect that they changed. See | ||
| // HoodieSource#checkBoundedCommitRangeUnchanged. | ||
| private final Option<String> readStartCommit; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 nit: could you rename |
||
| private final Option<String> readEndCommit; | ||
| // Deferred commit-range failure, present only when this enumerator was restored from a checkpoint | ||
| // whose range differs from the configured one. Raised from start() rather than at restore time, | ||
| // because on the initial restore from a savepoint the coordinator context is not yet initialized | ||
| // and its failJob cannot terminate the job. See HoodieSource#checkBoundedCommitRangeUnchanged. | ||
| private final Option<String> rangeFailure; | ||
|
|
||
| public HoodieStaticSplitEnumerator( | ||
| String tableName, SplitEnumeratorContext<HoodieSourceSplit> enumeratorContext, HoodieSplitProvider provider) { | ||
| String tableName, | ||
| SplitEnumeratorContext<HoodieSourceSplit> enumeratorContext, | ||
| HoodieSplitProvider provider, | ||
| Option<String> readStartCommit, | ||
| Option<String> readEndCommit, | ||
| Option<String> rangeFailure) { | ||
| super(tableName, enumeratorContext, provider); | ||
| this.readStartCommit = readStartCommit; | ||
| this.readEndCommit = readEndCommit; | ||
| this.rangeFailure = rangeFailure; | ||
| } | ||
|
|
||
| /** | ||
| * Raises the deferred commit-range failure before starting split discovery. Flink's | ||
| * single-threaded coordinator executor guarantees this runs before any other enumerator callback, | ||
| * and by this point {@code OperatorCoordinatorHolder#start()} has asserted that the coordinator | ||
| * context is initialized, so the throw reaches {@code context.failJob} and terminates the job. | ||
| * {@link SuppressRestartsException} keeps the restart strategy from looping on it: the same | ||
| * mismatch would recur on every restore from the same checkpoint. | ||
| */ | ||
| @Override | ||
| public void start() { | ||
| if (rangeFailure.isPresent()) { | ||
| log.error(rangeFailure.get()); | ||
| throw new SuppressRestartsException(new HoodieException(rangeFailure.get())); | ||
| } | ||
| super.start(); | ||
| } | ||
|
|
||
| @Override | ||
| public HoodieSplitEnumeratorState snapshotState(long checkpointId) { | ||
| return new HoodieSplitEnumeratorState( | ||
| splitProvider.state(), Option.empty(), Option.empty(), readStartCommit, readEndCommit); | ||
| } | ||
|
|
||
| @Override | ||
|
|
||
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.
Bumping the outer state serializer to v2 also makes the unchanged call below use
splitSerializer.deserialize(2, splitBytes), even though these bytes are still produced byHoodieSourceSplitSerializerv1. The nested deserializer currently ignores its version argument, so the tests pass, but this silently couples the two independent formats and will select the wrong split format as soon as that deserializer becomes version-aware. Could we record the nested serializer version with each payload (or explicitly map outer v1/v2 to split v1) instead of forwarding the outer version?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 — done. v2 payloads now lead with splitSerializer.getVersion() and that is what's passed to splitSerializer.deserialize(...); v1 payloads map to LEGACY_SPLIT_SERIALIZER_VERSION = 1. Also added a version > VERSION guard.