jojochuang commented on code in PR #8678:
URL: https://github.com/apache/ozone/pull/8678#discussion_r2196381889


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -769,6 +770,15 @@ public static String getSnapshotPath(OzoneConfiguration 
conf,
         OM_DB_NAME + checkpointDirName;
   }
 
+  public static String extractSnapshotIDFromCheckpointDirName(String 
snapshotPath) {

Review Comment:
   this public static method does not have test coverage, is only used by 
OMDBCheckpointServletInodeBasedXfer, and does not use any internal variables or 
method inside OmSnapshotManager. It should be moved into 
OMDBCheckpointServletInodeBasedXfer as a private method.
   
   Otherwise if it is intended to be used as is later, please add javadoc and 
test coverage.



##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/codec/OMDBDefinition.java:
##########
@@ -358,5 +360,13 @@ public String getName() {
   public String getLocationConfigKey() {
     return OMConfigKeys.OZONE_OM_DB_DIRS;
   }
+
+  public static List<String> getAllColumnFamilies() {

Review Comment:
   ```suggestion
     /**
     * Returns a list of all column family names defined in the database.
     *
     * @return list of column family names
     */
     public static List<String> getAllColumnFamilies() {
   ```



##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMDbCheckpointServletInodeBasedXfer.java:
##########
@@ -278,13 +276,150 @@ void testContentsOfTarballWithSnapshot() throws 
Exception {
     assertFalse(hardlinkFilePath.toFile().exists());
   }
 
+  /**
+   * Verifies that a manually added entry to the snapshot's delete table
+   * is persisted and can be retrieved from snapshot db loaded from OM DB 
checkpoint.
+   */
+  @Test
+  public void testSnapshotDBConsistency() throws Exception {
+    String volumeName = "vol" + RandomStringUtils.secure().nextNumeric(5);
+    String bucketName = "buck" + RandomStringUtils.secure().nextNumeric(5);
+    AtomicReference<DBCheckpoint> realCheckpoint = new AtomicReference<>();
+    setupClusterAndMocks(volumeName, bucketName, realCheckpoint);
+    List<OzoneSnapshot> snapshots = new ArrayList<>();
+    client.getObjectStore().listSnapshot(volumeName, bucketName, "", null)
+        .forEachRemaining(snapshots::add);
+    OzoneSnapshot snapshotToModify = snapshots.get(0);
+    String dummyKey = "dummyKey";
+    writeDummyKeyToDeleteTableOfSnapshotDB(snapshotToModify, bucketName, 
volumeName, dummyKey);
+    // Get the tarball.
+    omDbCheckpointServletMock.doGet(requestMock, responseMock);
+    String testDirName = folder.resolve("testDir").toString();
+    String newDbDirName = testDirName + OM_KEY_PREFIX + OM_DB_NAME;
+    File newDbDir = new File(newDbDirName);
+    assertTrue(newDbDir.mkdirs());
+    FileUtil.unTar(tempFile, newDbDir);
+    Set<Path> allPathsInTarball = getAllPathsInTarball(newDbDir);
+    // create hardlinks now
+    OmSnapshotUtils.createHardLinks(newDbDir.toPath());
+    for (Path old : allPathsInTarball) {
+      assertTrue(old.toFile().delete());
+    }
+    Path snapshotDbDir = Paths.get(newDbDir.toPath().toString(), 
OM_SNAPSHOT_CHECKPOINT_DIR,
+        OM_DB_NAME + "-" + snapshotToModify.getSnapshotId());
+    deleteWalFiles(snapshotDbDir);
+    assertTrue(Files.exists(snapshotDbDir));
+    String value = getValueFromSnapshotDeleteTable(dummyKey, 
snapshotDbDir.toString());
+    assertNotNull(value);
+  }
+
+  private static void deleteWalFiles(Path snapshotDbDir) throws IOException {
+    try (Stream<Path> filesInTarball = Files.list(snapshotDbDir)) {
+      List<Path> files = filesInTarball.filter(p -> 
p.toString().contains(".log"))
+          .collect(Collectors.toList());
+      for (Path p : files) {
+        Files.delete(p);
+      }
+    }
+  }
+
+  private static Set<Path> getAllPathsInTarball(File newDbDir) throws 
IOException {
+    Set<Path> allPathsInTarball = new HashSet<>();
+    try (Stream<Path> filesInTarball = Files.list(newDbDir.toPath())) {
+      List<Path> files = filesInTarball.collect(Collectors.toList());
+      for (Path p : files) {
+        File file = p.toFile();
+        if (file.getName().equals(OmSnapshotManager.OM_HARDLINK_FILE)) {
+          continue;
+        }
+        allPathsInTarball.add(p);
+      }
+    }
+    return allPathsInTarball;
+  }
+
+  private void writeDummyKeyToDeleteTableOfSnapshotDB(OzoneSnapshot 
snapshotToModify, String bucketName,
+      String volumeName, String keyName)
+      throws IOException {
+    try (UncheckedAutoCloseableSupplier<OmSnapshot> supplier = 
om.getOmSnapshotManager()
+        .getSnapshot(snapshotToModify.getSnapshotId())) {
+      OmSnapshot omSnapshot = supplier.get();
+      OmKeyInfo dummyOmKeyInfo =
+          new 
OmKeyInfo.Builder().setBucketName(bucketName).setVolumeName(volumeName).setKeyName(keyName)
+              
.setReplicationConfig(StandaloneReplicationConfig.getInstance(ONE)).build();
+      RepeatedOmKeyInfo dummyRepeatedKeyInfo =
+          new 
RepeatedOmKeyInfo.Builder().setOmKeyInfos(Collections.singletonList(dummyOmKeyInfo)).build();
+      
omSnapshot.getMetadataManager().getDeletedTable().put(dummyOmKeyInfo.getKeyName(),
 dummyRepeatedKeyInfo);
+    }
+  }
+
+  private void setupClusterAndMocks(String volumeName, String bucketName,
+      AtomicReference<DBCheckpoint> realCheckpoint) throws Exception {
+    setupCluster();
+    setupMocks();
+    om.getKeyManager().getSnapshotSstFilteringService().pause();
+    
when(requestMock.getParameter(OZONE_DB_CHECKPOINT_INCLUDE_SNAPSHOT_DATA)).thenReturn("true");
+    // Create a "spy" dbstore keep track of the checkpoint.
+    writeData(volumeName, bucketName, true);
+    DBStore dbStore = om.getMetadataManager().getStore();
+    DBStore spyDbStore = spy(dbStore);
+    when(spyDbStore.getCheckpoint(true)).thenAnswer(b -> {
+      DBCheckpoint checkpoint = spy(dbStore.getCheckpoint(true));
+      // Don't delete the checkpoint, because we need to compare it
+      // with the snapshot data.
+      doNothing().when(checkpoint).cleanupCheckpoint();
+      realCheckpoint.set(checkpoint);
+      return checkpoint;
+    });
+    // Init the mock with the spyDbstore
+    doCallRealMethod().when(omDbCheckpointServletMock).initialize(any(), any(),
+        eq(false), any(), any(), eq(false));
+    omDbCheckpointServletMock.initialize(spyDbStore, 
om.getMetrics().getDBCheckpointMetrics(),
+        false,
+        om.getOmAdminUsernames(), om.getOmAdminGroups(), false);
+    when(responseMock.getOutputStream()).thenReturn(servletOutputStream);
+  }
+
+  String getValueFromSnapshotDeleteTable(String key, String snapshotDB) {

Review Comment:
   ```suggestion
     private String getValueFromSnapshotDeleteTable(String key, String 
snapshotDB) {
   ```



-- 
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: issues-unsubscr...@ozone.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@ozone.apache.org
For additional commands, e-mail: issues-h...@ozone.apache.org

Reply via email to