[GitHub] [bookkeeper] dlg99 commented on a change in pull request #1949: [bookie-gc] add option to cache entry-log metadata map into rocksDB

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

dlg99 commented on a change in pull request #1949:
URL: https://github.com/apache/bookkeeper/pull/1949#discussion_r581318927



##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java
##########
@@ -553,11 +589,14 @@ protected void compactEntryLog(EntryLogMetadata entryLogMeta) {
             LOG.info("Extracting entry log meta from entryLogId: {}", entryLogId);
 
             try {
-                // Read through the entry log file and extract the entry log meta
+                // Read through the entry log file and extract the entry log
+                // meta
                 EntryLogMetadata entryLogMeta = entryLogger.getEntryLogMetadata(entryLogId);
                 removeIfLedgerNotExists(entryLogMeta);
                 if (entryLogMeta.isEmpty()) {
                     entryLogger.removeEntryLog(entryLogId);
+                    // remove it from entrylogmetadata-map if it presents into map

Review comment:
       nit: "remove it from entrylogmetadata-map if it is present in the map"

##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java
##########
@@ -127,6 +126,20 @@ public GarbageCollectorThread(ServerConfiguration conf, LedgerManager ledgerMana
                 Executors.newSingleThreadScheduledExecutor(new DefaultThreadFactory("GarbageCollectorThread")));
     }
 
+    private EntryLogMetadataMap createEntryLogMetadataMap() throws IOException {
+        if (conf.isGcEntryLogMetadataCacheEnabled()) {
+            String baseDir = this.conf.getGcEntryLogMetadataCachePath();
+            try {
+                return new PersistentEntryLogMetadataMap(baseDir, conf);

Review comment:
       Please consider  adding tests for such scenarios (bookie stop, RocksDB corruption, bookie starts) (persistent to in-memory to persistent switch, GC runs successfully before and after, collects everything, does not collect i.e. entry logs added while using in-memory). Sorry if I missed these in the existing tests, I am still going through the code

##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogMetadata.java
##########
@@ -96,4 +109,111 @@ public String toString() {
         return sb.toString();
     }
 
+    /**
+     * Serializes {@link EntryLogMetadata} and writes to
+     * {@link DataOutputStream}.
+     * <pre>
+     * schema:
+     * 2-bytes: schema-version
+     * 8-bytes: entrylog-entryLogId
+     * 8-bytes: entrylog-totalSize
+     * 8-bytes: entrylog-remainingSize
+     * 8-bytes: total number of ledgers
+     * ledgers-map
+     * [repeat]: (8-bytes::ledgerId, 8-bytes::size-of-ledger)
+     * </pre>
+     * @param out
+     * @throws IOException
+     *             throws if it couldn't serialize metadata-fields
+     * @throws IllegalStateException
+     *             throws if it couldn't serialize ledger-map
+     */
+    public void serialize(DataOutputStream out) throws IOException, IllegalStateException {
+        out.writeShort(DEFAULT_SERIALIZATION_VERSION);
+        out.writeLong(entryLogId);
+        out.writeLong(totalSize);
+        out.writeLong(remainingSize);
+        out.writeLong(ledgersMap.size());
+        ledgersMap.forEach((ledgerId, size) -> {
+            try {

Review comment:
       this try/catch should be around whole method body, i.e. `out.writeShort(DEFAULT_SERIALIZATION_VERSION)` as well as `out.flush()` can throw IOException too. 
   Overall, I'd either leave it at IOException and not catch anything (and pass IOException up) or catch everything and throw something like SerializationException

##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java
##########
@@ -127,6 +126,20 @@ public GarbageCollectorThread(ServerConfiguration conf, LedgerManager ledgerMana
                 Executors.newSingleThreadScheduledExecutor(new DefaultThreadFactory("GarbageCollectorThread")));
     }
 
+    private EntryLogMetadataMap createEntryLogMetadataMap() throws IOException {
+        if (conf.isGcEntryLogMetadataCacheEnabled()) {
+            String baseDir = this.conf.getGcEntryLogMetadataCachePath();
+            try {
+                return new PersistentEntryLogMetadataMap(baseDir, conf);

Review comment:
       Are you trying to preserve data on the disk between bookie restarts? 
   If not (I assume) it makes sense to explicitly clean up the baseDir and avoid situations like dealing with corrupt data, recovery from crashes, from config switches between persistent and in-memory metadata map, etc 




----------------------------------------------------------------
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.

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