-
Notifications
You must be signed in to change notification settings - Fork 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
[core] Fix race condition for earliest snapshot #4930
base: master
Are you sure you want to change the base?
[core] Fix race condition for earliest snapshot #4930
Conversation
Long earliest; | ||
if (startFromChangelog) { | ||
Long earliestChangelog = earliestLongLivedChangelogId(); | ||
earliest = earliestChangelog == null ? earliestSnapshot : earliestChangelog; | ||
earliest = earliestChangelog == null ? earliestSnapshotId : earliestChangelog; |
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.
earliestChangelogId
return null; | ||
} | ||
|
||
Long latest = null; |
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.
latestSnapshotId
try { | ||
earliestSnapshot = tryGetChangelogOrSnapshot(earliest); | ||
break; | ||
} catch (FileNotFoundException e) { |
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.
Add a log here?
do { | ||
try { | ||
return tryGetSnapshot(snapshotId); | ||
} catch (FileNotFoundException e) { |
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.
Add a log here?
@@ -163,11 +163,28 @@ public Snapshot tryGetSnapshot(long snapshotId) throws FileNotFoundException { | |||
return snapshot; | |||
} | |||
|
|||
private @Nullable Snapshot tryGetEarliestSnapshotLaterThanOrEqualTo( | |||
long snapshotId, long stopId) { |
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.
stopSnapshotId?
Purpose
Linked issue: close #3479
This PR fixes a racing bug about earliest snapshot. When thread A is trying to get the earliest snapshot of a table, while the snapshot is being expired by thread B, thread A might throw exception that it cannot get the snapshot information from the earliest snapshot id.
This PR fixes the bug by adding retrying logic to seek the second earliest snapshot if the first one cannot be found.
Note that, theoretically speaking, any snapshot (including the latest one) could encounter this issue, but the probability is quite small, so this PR only fixes the bug for invocations on the earliest snapshot.
Tests
Tests in SnapshotManagerTest are used to verify against the changes.
API and Format
This change does not affect API or storage format.
Documentation
This change does not introduce a new feature.