-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Add lastCommittedSnapshotId commit metric and document missing metrics #7589
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 |
|---|---|---|
|
|
@@ -155,6 +155,7 @@ public class FileStoreCommitImpl implements FileStoreCommit { | |
| private boolean ignoreEmptyCommit; | ||
| private CommitMetrics commitMetrics; | ||
| private boolean appendCommitCheckConflict = false; | ||
| private long lastCommittedSnapshotId = -1L; | ||
|
|
||
| public FileStoreCommitImpl( | ||
| SnapshotCommit snapshotCommit, | ||
|
|
@@ -371,7 +372,8 @@ public int commit(ManifestCommittable committable, boolean checkAppendFiles) { | |
| changes.compactChangelog, | ||
| commitDuration, | ||
| generatedSnapshot, | ||
| attempts); | ||
| attempts, | ||
| lastCommittedSnapshotId); | ||
| } | ||
| } | ||
| return generatedSnapshot; | ||
|
|
@@ -384,7 +386,8 @@ private void reportCommit( | |
| List<ManifestEntry> compactChangelogFiles, | ||
| long commitDuration, | ||
| int generatedSnapshots, | ||
| int attempts) { | ||
| int attempts, | ||
| long lastCommittedSnapshotId) { | ||
| CommitStats commitStats = | ||
| new CommitStats( | ||
| appendTableFiles, | ||
|
|
@@ -393,7 +396,8 @@ private void reportCommit( | |
| compactChangelogFiles, | ||
| commitDuration, | ||
| generatedSnapshots, | ||
| attempts); | ||
| attempts, | ||
| lastCommittedSnapshotId); | ||
| commitMetrics.reportCommit(commitStats); | ||
| } | ||
|
|
||
|
|
@@ -535,7 +539,8 @@ public int overwritePartition( | |
| emptyList(), | ||
| commitDuration, | ||
| generatedSnapshot, | ||
| attempts); | ||
| attempts, | ||
| lastCommittedSnapshotId); | ||
| } | ||
| } | ||
| return generatedSnapshot; | ||
|
|
@@ -801,6 +806,7 @@ CommitResult tryCommitOnce( | |
| if (snapshot.commitUser().equals(commitUser) | ||
| && snapshot.commitIdentifier() == identifier | ||
| && snapshot.commitKind() == commitKind) { | ||
| lastCommittedSnapshotId = snapshot.id(); | ||
| return new SuccessCommitResult(); | ||
| } | ||
| } | ||
|
|
@@ -1045,6 +1051,7 @@ CommitResult tryCommitOnce( | |
| if (strictModeChecker != null) { | ||
| strictModeChecker.update(newSnapshotId); | ||
| } | ||
| lastCommittedSnapshotId = newSnapshotId; | ||
|
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. This misses the duplicate-success retry path. In false-success retry, the earlier duplicate-check branch returns before this assignment, so the gauge can stay
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. Good catch, I have made changes to handle that. |
||
| CommitCallback.Context context = | ||
| new CommitCallback.Context( | ||
| finalBaseFiles, finalDeltaFiles, indexFiles, newSnapshot, identifier); | ||
|
|
||
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.
This metric has been added previously, but the docs doesn't reference it.