[GitHub] [ozone] smengcl commented on a diff in pull request #3786: HDDS-7281. [Snapshot] Handle RocksDB compaction DAG persistence and reconstruction

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

smengcl commented on code in PR #3786:
URL: https://github.com/apache/ozone/pull/3786#discussion_r991538396


##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdiff/RocksDBCheckpointDiffer.java:
##########
@@ -247,9 +300,30 @@ public void setRocksDBForCompactionTracking(
           public void onCompactionCompleted(
               final RocksDB db, final CompactionJobInfo compactionJobInfo) {
             synchronized (db) {
+
               LOG.warn(compactionJobInfo.compactionReason().toString());
               LOG.warn("List of input files:");
+
+              if (compactionJobInfo.inputFiles().size() == 0) {
+                LOG.error("Compaction input files list is empty?");
+                return;
+              }
+
+              final StringBuilder sb = new StringBuilder();
+
+              // kLevelL0FilesNum / kLevelMaxLevelSize. TODO: REMOVE
+              sb.append("# ").append(compactionJobInfo.compactionReason()).append('\n');
+
+              // Trim DB path, only keep the SST file name
+              final int filenameBegin =
+                  compactionJobInfo.inputFiles().get(0).lastIndexOf("/");
+
               for (String file : compactionJobInfo.inputFiles()) {
+                final String fn = file.substring(filenameBegin + 1);
+                sb.append(fn).append('\t');  // TODO: Trim last delimiter
+
+                // Create hardlink backups for the SST files that are going

Review Comment:
   Good point. Thanks George for checking out this WIP patch.
   
   I checked the RDB impl. And it seems indeed at least in the `TrivialMove` case SSTs could be deleted [here](https://github.com/facebook/rocksdb/blob/v7.4.5/db/db_impl/db_impl_compaction_flush.cc#L3299-L3301) in `DBImpl::BackgroundCompaction` before the `NotifyOnCompactionCompleted` [callback](https://github.com/facebook/rocksdb/blob/v7.4.5/db/db_impl/db_impl_compaction_flush.cc#L3446). On the other hand, we might not actually be interested in [`TrivialMove`](https://github.com/facebook/rocksdb/wiki/Compaction-Trivial-Move) since it seems to be just bumping up SST file compaction level (without even changing the SST file name?). But we do need to track if the file name changes. (Note: I did see input/output file name pair with the exact single file name in some runs with the current PR.)
   
   In the `NonTrivial` case, I am guessing input SSTs are also removed by `compaction_job` constructed [here](https://github.com/facebook/rocksdb/blob/v7.4.5/db/db_impl/db_impl_compaction_flush.cc#L3388-L3403) at some point (but not guaranteed?) after compaction completed, since we do see successful SST hardlinks created by the callback where the original SST doesn't exist in the original RDB directory anymore.
   
   But anyways, it does seem safer to retain the SSTs (by hardlinking them) in `onCompactionBegin` instead (guaranteed to get the call back from background compaction before the input SSTs are removed). I will patch this in a coming commit.
   
   
   I also have some pending changes that modifies how the log flush works. I should be able to push the commits to the PR in a day or two. (Will amend the PR description accordingly.)



-- 
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@ozone.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org