arunsarin85 commented on code in PR #11324:
URL: https://github.com/apache/ozone/pull/11324#discussion_r4120082897
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/test/java/org/apache/ozone/rocksdiff/TestRocksDBCheckpointDiffer.java:
##########
@@ -1065,12 +1055,75 @@ void diffAllSnapshots(RocksDBCheckpointDiffer differ)
assertThat(actualFiles).containsExactlyInAnyOrderElementsOf(expectedFiles);
}
}
- // Guard against getSSTDiffList silently returning nothing for every input.
+ assertThat(validatedSnapshotPairs)
+ .as("expected compaction DAG diffs for at least one snapshot pair")
+ .isPositive();
assertThat(sawNonEmptyDiff)
.as("expected at least one non-empty SST diff across snapshots")
.isTrue();
}
+ private Set<String> allTablesForDiff() {
+ Set<String> tables = new HashSet<>(COLUMN_FAMILIES_TO_TRACK_IN_DAG);
+ tables.add("compactionLogTable");
+ return tables;
+ }
+
+ private List<SstFileInfo> getTrackedSstFilesFromSnapshot(DifferSnapshotInfo
snap) {
+ return snap.getSstFiles(0, allTablesForDiff());
+ }
+
+ private static List<SstFileInfo> requireSstDiffList(
+ Optional<List<SstFileInfo>> diffList,
+ DifferSnapshotInfo src,
+ DifferSnapshotInfo dest) {
+ if (diffList.isPresent()) {
+ return diffList.get();
+ }
+ throw new AssertionError(String.format(
+ "getSSTDiffList returned empty Optional (DAG could not reach all
destination SSTs) "
+ + "from '%s' to '%s'", src.getDbPath(0), dest.getDbPath(0)));
+ }
+
+ private void assertCompactionSstBackups(RocksDBCheckpointDiffer differ)
throws IOException {
+ Set<String> tablesToLookup = allTablesForDiff();
+ DifferSnapshotInfo firstSnapshot = snapshots.get(0);
+ DifferSnapshotInfo lastSnapshot = snapshots.get(snapshots.size() - 1);
+ List<SstFileInfo> diffSinceFirst = requireSstDiffList(
+ differ.getSSTDiffList(
+ new DifferSnapshotVersion(lastSnapshot, 0, tablesToLookup),
+ new DifferSnapshotVersion(firstSnapshot, 0, tablesToLookup),
+ null, tablesToLookup, true),
+ lastSnapshot, firstSnapshot);
+ Set<String> lastSnapshotFileNames =
getTrackedSstFilesFromSnapshot(lastSnapshot).stream()
+ .map(SstFileInfo::getFileName)
+ .collect(Collectors.toSet());
+ Set<String> diffNotInLastSnapshot = diffSinceFirst.stream()
+ .map(SstFileInfo::getFileName)
+ .filter(name -> !lastSnapshotFileNames.contains(name))
+ .collect(Collectors.toSet());
+ ConcurrentMap<String, CompactionNode> compactionNodes =
differ.getCompactionNodeMap();
+ Set<String> backupBaseNames;
+ try (Stream<Path> sstPathStream = Files.list(sstBackUpDir.toPath())) {
+ List<Path> backupPaths = sstPathStream.collect(Collectors.toList());
+ backupBaseNames = backupPaths.stream()
+ .map(path -> getBaseName(path.getFileName().toString()))
+ .collect(Collectors.toSet());
+ assertThat(backupBaseNames).hasSizeGreaterThanOrEqualTo(7);
+ assertThat(backupBaseNames).allMatch(name -> name.matches("\\d+"));
Review Comment:
Done!
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/test/java/org/apache/ozone/rocksdiff/TestRocksDBCheckpointDiffer.java:
##########
@@ -1006,35 +997,36 @@ private static List<ColumnFamilyDescriptor>
getColumnFamilyDescriptors() {
void diffAllSnapshots(RocksDBCheckpointDiffer differ)
throws IOException {
final DifferSnapshotInfo src = snapshots.get(snapshots.size() - 1);
+ Set<String> allTables = allTablesForDiff();
+ int validatedSnapshotPairs = 0;
boolean sawNonEmptyDiff = false;
- for (DifferSnapshotInfo snap : snapshots) {
- // Returns a list of SST files to be fed into RocksCheckpointDiffer Dag.
- List<String> tablesToTrack = new
ArrayList<>(COLUMN_FAMILIES_TO_TRACK_IN_DAG);
- // Add some invalid index.
- tablesToTrack.add("compactionLogTable");
- // Baseline diff when tracking every table. A subset's diff must equal
- // this baseline filtered to the subset's column families (files with no
- // column family are always kept). This relationship is deterministic and
- // stable across RocksDB versions, unlike hard-coded SST file names.
- Set<String> allTables = new HashSet<>(tablesToTrack);
- List<SstFileInfo> baseline = differ.getSSTDiffList(
+ for (DifferSnapshotInfo snap : snapshots) {
+ Optional<List<SstFileInfo>> fullDagDiffOpt = differ.getSSTDiffList(
new DifferSnapshotVersion(src, 0, allTables),
new DifferSnapshotVersion(snap, 0, allTables),
- null, allTables, true).orElse(Collections.emptyList());
- sawNonEmptyDiff = sawNonEmptyDiff || !baseline.isEmpty();
-
- // Independent structural oracle, not derived from getSSTDiffList's own
- // output: a snapshot diffed against itself must have no differing SST
- // files. Together with the sawNonEmptyDiff guard below, this bounds a
- // systematically broken diff in both directions (returning nothing, or
- // returning files even for identical snapshots).
+ null, allTables, true);
+ if (!fullDagDiffOpt.isPresent()) {
+ LOG.info("Skipping DAG diff to '{}' because compaction DAG could not
reach all "
+ + "destination SST files", snap.getDbPath(0));
+ continue;
+ }
Review Comment:
full-table diff now uses requireSstDiffList so we fail fast like the subset
paths; removed validatedSnapshotPairs since the self-diff check still covers
the positive case.
--
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]
