[GitHub] [bookkeeper] RaulGracia commented on a change in pull request #2779: Issue 2728: Entry Log GC may get blocked when using entryLogPerLedgerEnabled option

[GitBox <gi...@apache.org>]

RaulGracia commented on a change in pull request #2779:
URL: https://github.com/apache/bookkeeper/pull/2779#discussion_r699167387



##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java
##########
@@ -583,12 +584,15 @@ protected void compactEntryLog(EntryLogMetadata entryLogMeta) {
      * @throws IOException
      */
     protected Map<Long, EntryLogMetadata> extractMetaFromEntryLogs(Map<Long, EntryLogMetadata> entryLogMetaMap) {
-        // Extract it for every entry log except for the current one.
-        // Entry Log ID's are just a long value that starts at 0 and increments
-        // by 1 when the log fills up and we roll to a new one.
-        long curLogId = entryLogger.getLeastUnflushedLogId();
+        // Entry Log ID's are just a long value that starts at 0 and increments by 1 when the log fills up and we roll
+        // to a new one. We scan entry logs as follows:
+        // - entryLogPerLedgerEnabled is false: Extract it for every entry log except for the current one (un-flushed).
+        // - entryLogPerLedgerEnabled is true: Scan all flushed entry logs up to the highest known id.
+        Supplier<Long> finalEntryLog = () -> conf.isEntryLogPerLedgerEnabled() ? entryLogger.getLastLogId() :

Review comment:
       Sure, I can do that, no worries. 
   
   But I have a more fundamental question about this change that I would like to hear from (all of) you. I will try to summarize it:
   - By default, entry logs are all flushed with the exception of the current one.
   - With `entryLogPerLedgerEnabled`, there could be multiple unflushed entry logs across the id space.
   
   Now, let's imagine that the Bookie restarts and there is unflushed data/metadata of entry logs. In the default case, this situation would impact only on the last entry log. In the case of `entryLogPerLedgerEnabled`, there could be multiple entry logs impacted (i.e., data was in memory but not synced to disk).
   
   The question is: when a Bookie starts and replays the journal, would it take care of all the unflushed entry log data irrespective of whether it belongs to the last entry log or multiple entry logs? 
   
   My understanding is that the replay of the journal takes care of all the unflushed data of all entry logs (but I would like to confirm this with you). If this is true, then I think that this PR is safe. In addition, our experiments inducing multiple Bookie restarts in purpose show that no unflushed entry log was wrongly garbage collected after the Bookie was up again.

##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java
##########
@@ -583,12 +584,15 @@ protected void compactEntryLog(EntryLogMetadata entryLogMeta) {
      * @throws IOException
      */
     protected Map<Long, EntryLogMetadata> extractMetaFromEntryLogs(Map<Long, EntryLogMetadata> entryLogMetaMap) {
-        // Extract it for every entry log except for the current one.
-        // Entry Log ID's are just a long value that starts at 0 and increments
-        // by 1 when the log fills up and we roll to a new one.
-        long curLogId = entryLogger.getLeastUnflushedLogId();
+        // Entry Log ID's are just a long value that starts at 0 and increments by 1 when the log fills up and we roll
+        // to a new one. We scan entry logs as follows:
+        // - entryLogPerLedgerEnabled is false: Extract it for every entry log except for the current one (un-flushed).
+        // - entryLogPerLedgerEnabled is true: Scan all flushed entry logs up to the highest known id.
+        Supplier<Long> finalEntryLog = () -> conf.isEntryLogPerLedgerEnabled() ? entryLogger.getLastLogId() :

Review comment:
       Thanks @Vanlightly, then I understand that this change is safe. 
   
   I also like the considerations regarding the refactoring, we have been also discussing this with @fpj. But I wonder if it would be better to first have `entryLogPerLedgerEnabled` option working correctly (goal of this PR), and think in parallel about the refactoring of GC logic and ledger storage separately, as it appears to be a larger change. Does it sound reasonable? 




-- 
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: issues-unsubscribe@bookkeeper.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org