-
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] Add more metric for vacuum #56234
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: sevev <[email protected]>
Signed-off-by: sevev <[email protected]>
Signed-off-by: sevev <[email protected]>
Signed-off-by: sevev <[email protected]>
@@ -406,6 +417,7 @@ private void waitResponse() { | |||
LakeTablet tablet = (LakeTablet) tablets.get(stat.tabletId); | |||
tablet.setDataSize(stat.dataSize); | |||
tablet.setRowCount(stat.numRows); | |||
tablet.setStorageSize(stat.storageSize); | |||
tablet.setDataSizeUpdateTime(collectStatTime); | |||
} | |||
} |
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 use of partition.getLastVacuumTime()
when filtering tablets instead of using vacuumVersion
You can modify the code like this:
- lakeTablet.getDataSizeUpdateTime() >= partition.getLastVacuumTime();
+ lakeTablet.getDataSizeUpdateTime() >= snapshot.vacuumVersion;
@@ -489,7 +497,8 @@ public String toString() { | |||
buffer.append("versionEpoch: ").append(versionEpoch).append("; "); | |||
buffer.append("versionTxnType: ").append(versionTxnType).append("; "); | |||
|
|||
buffer.append("storageDataSize: ").append(storageDataSize()).append("; "); | |||
buffer.append("dataSize: ").append(dataSize()).append("; "); | |||
buffer.append("storageSize: ").append(storageSize()).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:
Renaming the method storageDataSize()
to dataSize()
without updating all references to it could cause issues if storageDataSize()
is used elsewhere.
You can modify the code like this:
// Ensure that all references to storageDataSize() are updated, or retain the old method for backward compatibility.
public long getTabletMaxDataSize() {
return maxDataSize;
}
public long dataSize() { // formerly named storageDataSize
long dataSize = 0;
for (MaterializedIndex mIndex : getMaterializedIndices(IndexExtState.VISIBLE)) {
dataSize += mIndex.getDataSize();
}
return dataSize;
}
// Optionally, retain the original method with a deprecated annotation if used elsewhere:
// @Deprecated
// public long storageDataSize() {
// return dataSize();
// }
public long storageSize() {
long storageSize = 0;
for (MaterializedIndex mIndex : getMaterializedIndices(IndexExtState.VISIBLE)) {
storageSize += mIndex.getStorageSize();
}
return storageSize;
}
public long storageRowCount() {
long rowCount = 0;
for (MaterializedIndex mIndex : getMaterializedIndices(IndexExtState.VISIBLE)) {
rowCount += mIndex.getRowCount();
}
return rowCount;
}
public String toString() {
buffer.append("versionEpoch: ").append(versionEpoch).append("; ");
buffer.append("versionTxnType: ").append(versionTxnType).append("; ");
buffer.append("dataSize: ").append(dataSize()).append("; ");
buffer.append("storageSize: ").append(storageSize()).append("; ");
buffer.append("storageRowCount: ").append(storageRowCount()).append("; ");
buffer.append("storageReplicaCount: ").append(storageReplicaCount()).append("; ");
}
} | ||
return total_size; | ||
} | ||
|
||
} // namespace starrocks::lake 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:
Potential infinite loop due to incorrect update condition for 'version'.
You can modify the code like this:
StatusOr<int64_t> TabletManager::collect_tablet_storage_size(int64_t tablet_id, int64_t curr_version,
int64_t vacuum_version) {
auto root_dir = tablet_root_location(tablet_id);
auto data_dir = join_path(root_dir, lake::kSegmentDirectoryName);
auto version = curr_version;
int64_t total_size = 0;
ASSIGN_OR_RETURN(auto fs, FileSystem::CreateSharedFromString(root_dir));
while (version >= vacuum_version) {
auto res = get_tablet_metadata(tablet_id, version, false);
if (res.status().is_not_found()) {
break;
} else if (!res.ok()) {
return res.status();
}
auto metadata = std::move(res).value();
if (version == curr_version) {
for (const auto& rowset : metadata->rowsets()) {
total_size += rowset.data_size();
}
}
if (version > vacuum_version) {
for (const auto& rowset : metadata->compaction_inputs()) {
total_size += rowset.data_size();
}
for (const auto& file : metadata->orphan_files()) {
total_size += file.size();
}
for (auto& [_, file_meta] : metadata->delvec_meta().version_to_file()) {
total_size += file_meta.size();
}
for (const auto& sstable : metadata->sstable_meta().sstables()) {
total_size += sstable.filesize();
}
}
// Update 'version' correctly to avoid potential infinite loop
int64_t new_version = metadata->prev_garbage_version();
if (new_version >= version) {
break; // Prevent infinite loop
}
version = new_version;
}
return total_size;
}
Signed-off-by: sevev <[email protected]>
|
[Java-Extensions Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
Why I'm doing:
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: