chihsuan commented on code in PR #11324:
URL: https://github.com/apache/ozone/pull/11324#discussion_r4118192129
##########
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:
Could we use `requireSstDiffList` here too? The subset diffs fail fast, but
this one skips, so most pairs could go unchecked. That would also let us drop
`validatedSnapshotPairs`, which the self-diff always keeps positive. I might be
missing a case where empty is expected here though.
##########
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:
Should we keep the `.sst` check from master here? I noticed `getBaseName`
strips the extension first, so a non-SST file could still pass. Copilot raised
this on
[#10890](https://github.com/apache/ozone/pull/10890#discussion_r3669115935) too.
```suggestion
assertThat(backupPaths).allMatch(path ->
path.getFileName().toString().matches("\\d+\\.sst"));
```
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/test/java/org/apache/ozone/rocksdiff/TestRocksDBCheckpointDiffer.java:
##########
@@ -956,12 +957,14 @@ public void testGetSSTDiffListWithoutDB(String
description,
* Does actual DB write, flush, compaction.
*/
@Test
- @Flaky("HDDS-15209")
void testDifferWithDB() throws Exception {
writeKeysAndCheckpointing();
readRocksDBInstance(ACTIVE_DB_DIR_NAME, activeRocksDB, null,
rocksDBCheckpointDiffer);
+ GenericTestUtils.waitFor((BooleanSupplier) () ->
+ rocksDBCheckpointDiffer.getInflightCompactions().isEmpty(), 1000,
10000);
Review Comment:
nit: Is the `(BooleanSupplier)` cast needed? `waitFor` has a single
overload, and it fits on one line without it. The import could go too.
```suggestion
GenericTestUtils.waitFor(() ->
rocksDBCheckpointDiffer.getInflightCompactions().isEmpty(), 1000, 10000);
```
##########
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;
+ }
+ validatedSnapshotPairs++;
+ List<SstFileInfo> fullDagDiff = fullDagDiffOpt.get();
+ sawNonEmptyDiff = sawNonEmptyDiff || !fullDagDiff.isEmpty();
+
+ // Independent structural oracle: a snapshot diffed against itself must
have no
+ // differing SST files. Together with sawNonEmptyDiff below, this bounds
a broken
+ // diff in both directions (returning nothing, or returning files for
identical snapshots).
if (snap == src) {
- assertThat(baseline)
+ assertThat(fullDagDiff)
.as("diff of a snapshot against itself must be empty")
.isEmpty();
}
+ List<String> tablesToTrack = new
ArrayList<>(COLUMN_FAMILIES_TO_TRACK_IN_DAG);
+ tablesToTrack.add("compactionLogTable");
Review Comment:
nit: Could we reuse `allTables` here? It builds the same four tables as
`allTablesForDiff()`, so the list could drift from the set later.
```suggestion
List<String> tablesToTrack = new ArrayList<>(allTables);
```
--
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]
