hemantk-12 commented on code in PR #5511:
URL: https://github.com/apache/ozone/pull/5511#discussion_r1389143086
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdiff/RocksDBCheckpointDiffer.java:
##########
@@ -188,6 +178,8 @@ public class RocksDBCheckpointDiffer implements
AutoCloseable,
private ColumnFamilyHandle compactionLogTableCFHandle;
private RocksDB activeRocksDB;
Review Comment:
Added a comment.
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdiff/RocksDBCheckpointDiffer.java:
##########
@@ -1531,50 +1533,79 @@ public void pngPrintMutableGraph(String filePath,
GraphType graphType)
graph.generateImage(filePath);
}
- private List<CompactionFileInfo> toFileInfoList(
- List<String> sstFiles,
- ManagedOptions options,
- ManagedReadOptions readOptions
+ /**
+ * Returns true is compactionFileInfo has column family information and
+ * doesn't belong to any of the COLUMN_FAMILIES_TO_TRACK_IN_DAG.
+ * Otherwise, false and should be added compaction DAG.
+ * CompactionFileInfo doesn't have column family information in two cases,
+ * i). Backward compatibility. Before HDDS-8940, column family is not
+ * persisted.
+ * ii). In case, it fails to read SST in {@link this.toFileInfo()}.
+ */
+ @VisibleForTesting
+ boolean shouldSkipFile(CompactionFileInfo compactionFileInfo) {
+ return Objects.nonNull(compactionFileInfo.getColumnFamily()) &&
+ !COLUMN_FAMILIES_TO_TRACK_IN_DAG.contains(
+ compactionFileInfo.getColumnFamily());
+ }
+
+ private List<CompactionFileInfo> toFileInfoList(List<String> sstFiles,
+ ManagedOptions options,
+ ManagedReadOptions
readOptions
) {
if (CollectionUtils.isEmpty(sstFiles)) {
return Collections.emptyList();
}
- final int fileNameOffset = sstFiles.get(0).lastIndexOf("/") + 1;
List<CompactionFileInfo> response = new ArrayList<>();
for (String sstFile : sstFiles) {
- String fileName = sstFile.substring(fileNameOffset,
- sstFile.length() - SST_FILE_EXTENSION_LENGTH);
- CompactionFileInfo.Builder fileInfoBuilder =
- new CompactionFileInfo.Builder(fileName);
- SstFileReader fileReader = new SstFileReader(options);
- try {
- fileReader.open(sstFile);
- String columnFamily = StringUtils.bytes2String(
- fileReader.getTableProperties().getColumnFamilyName());
- SstFileReaderIterator iterator = fileReader.newIterator(readOptions);
- iterator.seekToFirst();
- String startKey = StringUtils.bytes2String(iterator.key());
- iterator.seekToLast();
- String endKey = StringUtils.bytes2String(iterator.key());
- fileInfoBuilder.setStartRange(startKey)
- .setEndRange(endKey)
- .setColumnFamily(columnFamily);
- } catch (RocksDBException rocksDBException) {
- // Ideally it should not happen. If it does just log the exception.
- // And let the compaction complete without the exception.
- // Throwing exception in compaction listener could fail the RocksDB.
- // In case of exception, compaction node will be missing start key,
- // end key and column family. And it will continue the traversal as
- // it was before HDDS-8940.
- LOG.warn("Failed to read SST file: {}.", sstFile, rocksDBException);
+ CompactionFileInfo fileInfo = toFileInfo(sstFile, options, readOptions);
+ if (shouldSkipFile(fileInfo)) {
+ LOG.debug("Skipping sst file: '{}' belongs to column family '{}'.",
Review Comment:
You are right. Removed this check altogether.
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdiff/RocksDBCheckpointDiffer.java:
##########
@@ -469,23 +458,38 @@ public void onCompactionBegin(RocksDB db,
return;
}
}
+ createHardLinks(compactionJobInfo.inputFiles());
+ }
+ };
+ }
- // Create hardlink backups for the SST files that are going
- // to be deleted after this RDB compaction.
- for (String file : compactionJobInfo.inputFiles()) {
- LOG.debug("Creating hard link for '{}'", file);
+ @VisibleForTesting
+ void createHardLinks(List<String> sourceFiles) {
+ try (ManagedOptions options = new ManagedOptions();
+ ManagedReadOptions readOptions = new ManagedReadOptions()) {
+ // Create hardlink backups for the SST files that are going
+ // to be deleted after this RDB compaction.
+ for (String file : sourceFiles) {
+ CompactionFileInfo fileInfo =
+ toFileInfo(file, options, readOptions);
+ if (shouldSkipFile(fileInfo)) {
Review Comment:
Removed this check.
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdiff/RocksDBCheckpointDiffer.java:
##########
@@ -455,9 +447,6 @@ public void onCompactionBegin(RocksDB db,
return;
}
Review Comment:
It is a great point. Thank you for the suggestion.
Removed individual `shouldSkipFile()` check and added early return in
compactionListeners if compaction job doesn't belong to desired column family.
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdiff/RocksDBCheckpointDiffer.java:
##########
@@ -188,6 +178,8 @@ public class RocksDBCheckpointDiffer implements
AutoCloseable,
private ColumnFamilyHandle compactionLogTableCFHandle;
private RocksDB activeRocksDB;
+ public static final Set<String> COLUMN_FAMILIES_TO_TRACK_IN_DAG =
Review Comment:
It is used in TestOmSnapshot.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
