-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
[Enhancement] Some enhancements of data file vacuuming for cloud native partition #50342
[Enhancement] Some enhancements of data file vacuuming for cloud native partition #50342
Conversation
@@ -387,4 +398,4 @@ private void waitResponse() { | |||
} | |||
} | |||
} | |||
} | |||
} No newline at end of file |
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.
The most risky bug in this code is:
Incorrect condition for removing tablets
You can modify the code like this:
snapshot.tablets.removeIf(t -> ((LakeTablet) t).getDataSizeUpdateTime() < visibleVersionTime &&
((LakeTablet) t).getDataSizeUpdateTime() <= lastSuccVacuumTime);
@@ -508,7 +547,7 @@ public String toString() { | |||
buffer.append("versionEpoch: ").append(versionEpoch).append("; "); | |||
buffer.append("versionTxnType: ").append(versionTxnType).append("; "); | |||
|
|||
buffer.append("storageDataSize: ").append(storageDataSize()).append("; "); | |||
buffer.append("storageDataSize: ").append(getDataSize()).append("; "); | |||
buffer.append("storageRowCount: ").append(storageRowCount()).append("; "); | |||
buffer.append("storageReplicaCount: ").append(storageReplicaCount()).append("; "); | |||
|
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.
The most risky bug in this code is:
Incorrect variable name used in the setLastSuccVacuumTime
method.
You can modify the code like this:
@@ -176,13 +177,42 @@ public boolean isImmutable() {
}
@Override
- public long getLastVacuumTime() {
- return lastVacuumTime;
+ public long getLastSuccVacuumTime() {
+ return lastSuccVacuumTime;
}
@Override
- public void setLastVacuumTime(long lastVacuumTime) {
- this.lastVacuumTime = lastVacuumTime;
+ public void setLastSuccVacuumTime(long lastSuccVacuumTime) {
+ this.lastSuccVacuumTime = lastSuccVacuumTime;
+ }
return true; | ||
} | ||
|
||
return false; | ||
} | ||
|
||
public long getMinRetainVersion() { |
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.
The most risky bug in this code is:
Potential logging null pointer when name
is not initialized in the shouldVacuum
method.
You can modify the code like this:
if ((storageSize - dataSize >= 50L * 1024 * 1024) && (magnification > 0.1)) {
LOG.debug("Partition: {}, storage size: {}, data size: {}, magnification: {} should vacuum now",
getName(), getStorageSize(), getDataSize(), magnification);
return true;
}
39546db
to
a8a39ce
Compare
c3338af
to
894eb32
Compare
Quality Gate failedFailed conditions |
[FE Incremental Coverage Report]✅ pass : 76 / 95 (80.00%) file detail
|
…ve partition 1. Add storage size property for cloud native partition 2. Start vacuum according to partition data size and storage size on S3 3. Add more vacuuming metrics on FE Signed-off-by: tracymacding <[email protected]>
894eb32
to
d40d5c9
Compare
Quality Gate passedIssues Measures |
[Java-Extensions Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
[BE Incremental Coverage Report]✅ pass : 49 / 61 (80.33%) file detail
|
Why I'm doing:
To improve data file vacuuming for cloud native table, some enhancements made in this pr:
What I'm doing:
Fixes #issue
What type of PR is this:
Does this PR entail a change in behavior?
If yes, please specify the type of change:
Checklist:
Bugfix cherry-pick branch check: