dlg99 commented on code in PR #3965:
URL: https://github.com/apache/bookkeeper/pull/3965#discussion_r1205015481
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java:
##########
@@ -755,8 +759,9 @@ protected void extractMetaFromEntryLogs() throws
EntryLogMetadataMapException {
// 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(entryLogMeta.getTotalSize());
+ if (removeEntryLog(entryLogId)) {
+
gcStats.getReclaimedSpaceViaDeletes().addCount(entryLogMeta.getTotalSize());
+ }
Review Comment:
we should increment some stats (reclaimFailedToDelete) in case of error/else
block to enable monitoring of such cases
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java:
##########
@@ -497,8 +497,9 @@ private void doGcEntryLogs() throws
EntryLogMetadataMapException {
// 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());
+ if (removeEntryLog(entryLogId)) {
+
gcStats.getReclaimedSpaceViaDeletes().addCount(meta.getTotalSize());
Review Comment:
we should increment some stats (reclaimFailedToDelete) in case of error/else
block to enable monitoring of such cases
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/DefaultEntryLogger.java:
##########
@@ -528,6 +528,7 @@ public boolean removeEntryLog(long entryLogId) {
}
if (!entryLogFile.delete()) {
Review Comment:
above we return false in case of file not found
```java
} catch (FileNotFoundException e) {
LOG.error("Trying to delete an entryLog file that could not be
found: "
+ entryLogId + ".log");
return false;
}
```
Would it make sense to simply log and return true; after all, we wanted to
delete the entry log, and let the `entryLogMetaMap.remove` run.
Alternatively, throw IllegalStateException to force an investigation into
how we got into such a state (is there a concurrent process that could delete
the entry log?) though this feels like an overkill.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
