Skip to content

HDDS-11908. Snapshot diff DAG traversal should not skip node based on prefix presence #7567

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
swamirishi merged 4 commits into from
Dec 13, 2024
Merged

HDDS-11908. Snapshot diff DAG traversal should not skip node based on prefix presence #7567

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
81423f1
HDDS-11908. Snapshot diff DAG traversal should not skip node based on…
swamirishi Dec 12, 2024
c4b5a3c
HDDS-11908. Fix findbugs
swamirishi Dec 13, 2024
3a29fea
Merge remote-tracking branch 'apache/master' into HEAD
swamirishi Dec 13, 2024
efa38b0
Merge remote-tracking branch 'apache/master' into HEAD
swamirishi Dec 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 0 additions & 45 deletions ...b-checkpoint-differ/src/main/java/org/apache/ozone/rocksdiff/RocksDBCheckpointDiffer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@

import com.google.protobuf.InvalidProtocolBufferException;
import org.apache.commons.collections.CollectionUtils;
import org.apache.commons.collections.MapUtils;
import org.apache.commons.lang3.tuple.Pair;
import org.apache.hadoop.hdds.StringUtils;
import org.apache.hadoop.hdds.conf.ConfigurationSource;
Expand Down Expand Up @@ -944,10 +943,6 @@ synchronized void internalGetSSTDiffList(
Preconditions.checkArgument(sameFiles.isEmpty(), "Set must be empty");
Preconditions.checkArgument(differentFiles.isEmpty(), "Set must be empty");

// Use source snapshot's table prefix. At this point Source and target's
// table prefix should be same.
Map<String, String> columnFamilyToPrefixMap = src.getTablePrefixes();

for (String fileName : srcSnapFiles) {
if (destSnapFiles.contains(fileName)) {
LOG.debug("Source '{}' and destination '{}' share the same SST '{}'",
Expand Down Expand Up @@ -1011,15 +1006,6 @@ synchronized void internalGetSSTDiffList(
}

for (CompactionNode nextNode : successors) {
if (shouldSkipNode(nextNode, columnFamilyToPrefixMap)) {
LOG.debug("Skipping next node: '{}' with startKey: '{}' and " +
"endKey: '{}' because it doesn't have keys related to " +
"columnFamilyToPrefixMap: '{}'.",
nextNode.getFileName(), nextNode.getStartKey(),
nextNode.getEndKey(), columnFamilyToPrefixMap);
continue;
}

if (sameFiles.contains(nextNode.getFileName()) ||
differentFiles.contains(nextNode.getFileName())) {
LOG.debug("Skipping known processed SST: {}",
Expand Down Expand Up @@ -1541,35 +1527,4 @@ private CompactionFileInfo toFileInfo(String sstFile,
return fileInfoBuilder.build();
}

@VisibleForTesting
boolean shouldSkipNode(CompactionNode node,
Map<String, String> columnFamilyToPrefixMap) {
// This is for backward compatibility. Before the compaction log table
// migration, startKey, endKey and columnFamily information is not persisted
// in compaction log files.
// Also for the scenario when there is an exception in reading SST files
// for the file node.
if (node.getStartKey() == null || node.getEndKey() == null ||
node.getColumnFamily() == null) {
LOG.debug("Compaction node with fileName: {} doesn't have startKey, " +
"endKey and columnFamily details.", node.getFileName());
return false;
}

if (MapUtils.isEmpty(columnFamilyToPrefixMap)) {
LOG.debug("Provided columnFamilyToPrefixMap is null or empty.");
return false;
}

if (!columnFamilyToPrefixMap.containsKey(node.getColumnFamily())) {
LOG.debug("SstFile node: {} is for columnFamily: {} while filter map " +
"contains columnFamilies: {}.", node.getFileName(),
node.getColumnFamily(), columnFamilyToPrefixMap.keySet());
return true;
}

String keyPrefix = columnFamilyToPrefixMap.get(node.getColumnFamily());
return !RocksDiffUtils.isKeyWithPrefixPresent(keyPrefix, node.getStartKey(),
node.getEndKey());
}
}
33 changes: 33 additions & 0 deletions ...ds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdiff/RocksDiffUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
*/
package org.apache.ozone.rocksdiff;

import com.google.common.annotations.VisibleForTesting;
import org.apache.commons.collections.MapUtils;
import org.apache.commons.lang3.StringUtils;
import org.apache.hadoop.hdds.utils.db.managed.ManagedOptions;
import org.apache.hadoop.hdds.utils.db.managed.ManagedReadOptions;
Expand Down Expand Up @@ -113,4 +115,35 @@ public static boolean doesSstFileContainKeyRange(String filepath,
}


@VisibleForTesting
Copy link
Contributor

@hemantk-12 hemantk-12 Dec 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this visibility tag.

Copy link
Contributor Author

@swamirishi swamirishi Dec 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is visible for testing since this shouldSkipNode function is being used only by tests and a function inside RocksDiffUtils. We could have made it private if not for the tests.

static boolean shouldSkipNode(CompactionNode node,
Map<String, String> columnFamilyToPrefixMap) {
// This is for backward compatibility. Before the compaction log table
// migration, startKey, endKey and columnFamily information is not persisted
// in compaction log files.
// Also for the scenario when there is an exception in reading SST files
// for the file node.
if (node.getStartKey() == null || node.getEndKey() == null ||
node.getColumnFamily() == null) {
LOG.debug("Compaction node with fileName: {} doesn't have startKey, " +
"endKey and columnFamily details.", node.getFileName());
return false;
}

if (MapUtils.isEmpty(columnFamilyToPrefixMap)) {
LOG.debug("Provided columnFamilyToPrefixMap is null or empty.");
return false;
}

if (!columnFamilyToPrefixMap.containsKey(node.getColumnFamily())) {
LOG.debug("SstFile node: {} is for columnFamily: {} while filter map " +
"contains columnFamilies: {}.", node.getFileName(),
node.getColumnFamily(), columnFamilyToPrefixMap.keySet());
return true;
}

String keyPrefix = columnFamilyToPrefixMap.get(node.getColumnFamily());
return !isKeyWithPrefixPresent(keyPrefix, node.getStartKey(),
node.getEndKey());
}
}
4 changes: 2 additions & 2 deletions ...eckpoint-differ/src/test/java/org/apache/ozone/rocksdiff/TestRocksDBCheckpointDiffer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1895,7 +1895,7 @@ public void testShouldSkipNode(Map<String, String> columnFamilyToPrefixMap,
.getCompactionNodeMap().values().stream()
.sorted(Comparator.comparing(CompactionNode::getFileName))
.map(node ->
rocksDBCheckpointDiffer.shouldSkipNode(node,
RocksDiffUtils.shouldSkipNode(node,
Copy link
Contributor

@hemantk-12 hemantk-12 Dec 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests (testShouldSkipNode and testShouldSkipNodeEdgeCase) should be moved to TestRocksDiffUtils. If not part of this PR, create a follow-up task.

columnFamilyToPrefixMap))
.collect(Collectors.toList());

Expand Down Expand Up @@ -1932,7 +1932,7 @@ public void testShouldSkipNodeEdgeCase(

rocksDBCheckpointDiffer.loadAllCompactionLogs();

assertEquals(expectedResponse, rocksDBCheckpointDiffer.shouldSkipNode(node,
assertEquals(expectedResponse, RocksDiffUtils.shouldSkipNode(node,
columnFamilyToPrefixMap));
}

Expand Down
Loading