adoroszlai commented on code in PR #6069:
URL: https://github.com/apache/ozone/pull/6069#discussion_r1462909347


##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMRatisSnapshots.java:
##########
@@ -347,6 +347,15 @@ private void checkSnapshot(OzoneManager leaderOM, 
OzoneManager followerOM,
     Path leaderActiveDir = Paths.get(leaderMetaDir.toString(), OM_DB_NAME);
     Path leaderSnapshotDir =
         Paths.get(getSnapshotPath(leaderOM.getConfiguration(), snapshotInfo));
+
+    // Get list of live files on the leader.
+    RocksDB activeRocksDB = 
((RDBStore)leaderOM.getMetadataManager().getStore()).getDb().getManagedRocksDb()
+        .get();
+    List<String> liveSstFiles = new ArrayList<>();
+    // strip the leading "/".
+    liveSstFiles.addAll(activeRocksDB.getLiveFiles().files.stream().map(s -> 
s.substring(1)).collect(
+        Collectors.toList()));

Review Comment:
   nit: can use the result of `collect()` directly, no need for two lists.
   
   ```suggestion
       // strip the leading "/".
       List<String> liveSstFiles = activeRocksDB.getLiveFiles().files.stream()
           .map(s -> s.substring(1))
           .collect(Collectors.toList());
   ```
   
   (also, it can be a `Set`, since we don't need to consider duplicates)



##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMRatisSnapshots.java:
##########
@@ -358,7 +367,8 @@ private void checkSnapshot(OzoneManager leaderOM, 
OzoneManager followerOM,
           Path leaderActiveSST =
               Paths.get(leaderActiveDir.toString(), fileName);
           // Skip if not hard link on the leader
-          if (!leaderActiveSST.toFile().exists()) {
+          // First confirm it is live
+          if (!liveSstFiles.stream().anyMatch(s -> s.equals(fileName))) {

Review Comment:
   nit: can be simplified?
   
   ```suggestion
             if (!liveSstFiles.contains(fileName)) {
   ```



##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMRatisSnapshots.java:
##########
@@ -198,12 +198,12 @@ public void shutdown() {
     }
   }
 
-  @ParameterizedTest
-  @ValueSource(ints = {100})
   // tried up to 1000 snapshots and this test works, but some of the
   //  timeouts have to be increased.
-  @Unhealthy("HDDS-10059")
-  void testInstallSnapshot(int numSnapshotsToCreate, @TempDir Path tempDir) 
throws Exception {
+  private static int numSnapshotsToCreate = 100;

Review Comment:
   nit: can be final
   
   ```suggestion
     private static final int NUM_SNAPSHOTS_TO_CREATE = 100;
   ```
   
   (needs corresponding change in `testInstallSnapshot`)



-- 
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]

Reply via email to