Skip to content
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

Fix ReclaimedSpaceViaDeletes stats incorrect problem. #3906

Merged

Conversation

horizonzy
Copy link
Member

Descriptions of the changes in this PR:

private void doGcEntryLogs() throws EntryLogMetadataMapException {
// Get a cumulative count, don't update until complete
AtomicLong totalEntryLogSizeAcc = new AtomicLong(0L);
// Loop through all of the entry logs and remove the non-active ledgers.
entryLogMetaMap.forEach((entryLogId, meta) -> {
try {
boolean modified = removeIfLedgerNotExists(meta);
if (meta.isEmpty()) {
// This means the entry log is not associated with any active
// ledgers anymore.
// We can remove this entry log file now.
LOG.info("Deleting entryLogId {} as it has no active ledgers!", entryLogId);
removeEntryLog(entryLogId);
gcStats.getReclaimedSpaceViaDeletes().addCount(meta.getTotalSize());
} else if (modified) {
// update entryLogMetaMap only when the meta modified.
entryLogMetaMap.put(meta.getEntryLogId(), meta);
}
} catch (EntryLogMetadataMapException e) {
// Ignore and continue because ledger will not be cleaned up
// from entry-logger in this pass and will be taken care in next
// schedule task
LOG.warn("Failed to remove ledger from entry-log metadata {}", entryLogId, e);
}
totalEntryLogSizeAcc.getAndAdd(meta.getRemainingSize());
});
this.totalEntryLogSize = totalEntryLogSizeAcc.get();
this.numActiveEntryLogs = entryLogMetaMap.size();
}

At line 501, increases getReclaimedSpaceViaDeletes stats.

protected void extractMetaFromEntryLogs() throws EntryLogMetadataMapException {
for (long entryLogId : entryLogger.getFlushedLogIds()) {
// Comb the current entry log file if it has not already been extracted.
if (entryLogMetaMap.containsKey(entryLogId)) {
continue;
}
// check whether log file exists or not
// if it doesn't exist, this log file might have been garbage collected.
if (!entryLogger.logExists(entryLogId)) {
continue;
}
LOG.info("Extracting entry log meta from entryLogId: {}", entryLogId);
try {
// Read through the entry log file and extract the entry log meta
EntryLogMetadata entryLogMeta = entryLogger.getEntryLogMetadata(entryLogId, throttler);
removeIfLedgerNotExists(entryLogMeta);
if (entryLogMeta.isEmpty()) {
LOG.info("Entry log file {} is empty, delete it from disk.", Long.toHexString(entryLogId));
entryLogger.removeEntryLog(entryLogId);
// remove it from entrylogmetadata-map if it is present in
// the map
entryLogMetaMap.remove(entryLogId);
} else {
entryLogMetaMap.put(entryLogId, entryLogMeta);
}
} catch (IOException e) {
LOG.warn("Premature exception when processing " + entryLogId
+ " recovery will take care of the problem", e);
}
}
}

But at line 755, it didn't increases getReclaimedSpaceViaDeletes stats.

@horizonzy
Copy link
Member Author

rerun failure checks

// We can remove this entry log file now.
LOG.info("Deleting entryLogId {} as it has no active ledgers!", entryLogId);
removeEntryLog(entryLogId);
gcStats.getReclaimedSpaceViaDeletes().addCount(entryLogMeta.getTotalSize());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to update the stats because the entryLogMeta is empty and the entryLogMeta.getTotalSize() will be 0

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

public void removeLedgerIf(LongPredicate predicate) {
ledgersMap.removeIf((ledgerId, size) -> {
boolean shouldRemove = predicate.test(ledgerId);
if (shouldRemove) {
remainingSize -= size;
}
return shouldRemove;
});
}
,
it only reduces the remainingSize, the totalSize won't be reduce to 0.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sorry, it's my mistake.

Copy link
Member

@wenbingshen wenbingshen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice Catch!

@hangc0276 hangc0276 merged commit cad1436 into apache:master Apr 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants