rich7420 commented on code in PR #10890:
URL: https://github.com/apache/ozone/pull/10890#discussion_r3740286842
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/test/java/org/apache/ozone/rocksdiff/TestRocksDBCheckpointDiffer.java:
##########
@@ -1077,12 +1065,93 @@ 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());
+ }
+
+ /**
+ * Snapshot-only SST diff (same rules as {@code getSSTDiffList(...,
useCompactionDag=false)}).
+ */
+ private List<SstFileInfo> buildNonDagMetadataDiff(DifferSnapshotInfo srcSnap,
+ DifferSnapshotInfo destSnap, Set<String> tablesToLookup) {
+ Set<SstFileInfo> srcSstFileInfos = new HashSet<>(srcSnap.getSstFiles(0,
tablesToLookup));
+ Set<SstFileInfo> destSstFileInfos = new HashSet<>(destSnap.getSstFiles(0,
tablesToLookup));
+ Map<String, SstFileInfo> differentFiles = new HashMap<>();
+ for (SstFileInfo srcSstFileInfo : srcSstFileInfos) {
+ if (!destSstFileInfos.contains(srcSstFileInfo)) {
+ differentFiles.put(srcSstFileInfo.getFileName(), srcSstFileInfo);
+ }
+ }
+ for (SstFileInfo destSstFileInfo : destSstFileInfos) {
+ if (!srcSstFileInfos.contains(destSstFileInfo)) {
+ differentFiles.put(destSstFileInfo.getFileName(), destSstFileInfo);
+ }
+ }
+ return new ArrayList<>(differentFiles.values());
+ }
+
+ 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> backupBaseNames;
+ try (Stream<Path> sstPathStream = Files.list(sstBackUpDir.toPath())) {
+ backupBaseNames = sstPathStream.map(path ->
getBaseName(path.getFileName().toString()))
+ .collect(Collectors.toSet());
+ assertThat(backupBaseNames).isNotEmpty();
+ assertThat(backupBaseNames).allMatch(name -> name.matches("\\d+"));
+ }
+ for (SstFileInfo diffFile : diffSinceFirst) {
+ String fileName = diffFile.getFileName();
+ assertTrue(lastSnapshotFileNames.contains(fileName) ||
backupBaseNames.contains(fileName),
Review Comment:
Weaker than what it replaces (master: exactly 7 links, `\d{6}\.sst`).
Relaxing `\d{6}`→`\d+` is fair (SST number width varies), but `isNotEmpty`
drops the count, and `lastSnapshotFileNames.contains(f) ||
backupBaseNames.contains(f)` is met by the last-snapshot branch alone — it'd
pass even if the backup dir were near-empty. Keep a lower-bound count and
assert the backup dir actually holds the DAG-diff SSTs?
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/test/java/org/apache/ozone/rocksdiff/TestRocksDBCheckpointDiffer.java:
##########
@@ -1018,34 +1009,33 @@ 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");
+ List<SstFileInfo> metadataDiff = buildNonDagMetadataDiff(src, snap,
allTables);
+ List<SstFileInfo> metadataDiffViaApi = differ.getSSTDiffList(
+ new DifferSnapshotVersion(src, 0, allTables),
+ new DifferSnapshotVersion(snap, 0, allTables),
+ null, allTables, false).orElse(Collections.emptyList());
+
assertThat(metadataDiffViaApi).containsExactlyInAnyOrderElementsOf(metadataDiff);
Review Comment:
Is this oracle independent of the code under test? `buildNonDagMetadataDiff`
(L1089) does the symmetric diff of `getSstFiles(0, tables)`, and the
`useCompactionDag=false` branch it's checked against (`getSSTDiffList`, main
L830-846) does the same diff over the same `getSstFiles(0, tables)` with the
same `SstFileInfo.equals` — so both sides move together and a matching bug
still passes. It also never exercises the compaction DAG. Diff against an
independent source, or drop it and rely on the DAG-path checks?
--
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]
