hemantk-12 commented on code in PR #3980:
URL: https://github.com/apache/ozone/pull/3980#discussion_r1042781601
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RDBStore.java:
##########
@@ -139,10 +134,10 @@ public RDBStore(File dbFile, ManagedDBOptions dbOptions,
//create snapshot directory if does not exist.
snapshotsParentDir = Paths.get(dbLocation.getParent(),
- OM_SNAPSHOT_DIR).toString();
+ OM_SNAPSHOT_CHECKPOINT_DIR).toString();
Review Comment:
Comment says create snapshot directory but it is actually creating snapshot
checkpointing directory. Either `OM_SNAPSHOT_DIR` was updated to
`OM_SNAPSHOT_CHECKPOINT_DIR` mistakenly or comment needs update. Exception
message may need update accordingly.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -220,6 +241,13 @@ public static String getSnapshotPrefix(String
snapshotName) {
snapshotName + OM_KEY_PREFIX;
}
+ public static String getSnapshotPath(OzoneConfiguration conf,
+ SnapshotInfo snapshotInfo) {
Review Comment:
Alignment is a bit off.
Alignment is off for lots of function in this class. Please fix it.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java:
##########
@@ -82,4 +111,164 @@ public void init() throws ServletException {
allowedGroups,
om.isSpnegoEnabled());
}
+
+ @Override
+ public void writeDbDataToStream(DBCheckpoint checkpoint,
+ HttpServletRequest request,
+ OutputStream destination)
+ throws IOException, InterruptedException {
+ // Map of inodes to path.
+ Map<Object, Path> copyFiles = new HashMap<>();
+ // Map of link to path.
+ Map<Path, Path> hardLinkFiles = new HashMap<>();
+
+ getFilesForArchive(checkpoint, copyFiles, hardLinkFiles,
+ includeSnapshotData(request));
+
+ try (TarArchiveOutputStream archiveOutputStream =
+ new TarArchiveOutputStream(destination)) {
+ archiveOutputStream
+ .setLongFileMode(TarArchiveOutputStream.LONGFILE_POSIX);
+ writeFilesToArchive(copyFiles, hardLinkFiles, archiveOutputStream);
+ }
+ }
+
+ private void getFilesForArchive(DBCheckpoint checkpoint,
+ Map<Object, Path> copyFiles,
+ Map<Path, Path> hardLinkFiles,
+ boolean includeSnapshotData)
+ throws IOException {
+
+ // Get the active fs files.
+ Path dir = checkpoint.getCheckpointLocation();
+ processDir(dir, copyFiles, hardLinkFiles, new HashSet<>());
+
+ if (!includeSnapshotData) {
+ return;
+ }
+
+ // Get the snapshot files.
+ Set<Path> snapshotPaths = waitForSnapshotDirs(checkpoint);
+ Path snapshotDir = Paths.get(OMStorage.getOmDbDir(getConf()).toString(),
+ OM_SNAPSHOT_DIR);
+ processDir(snapshotDir, copyFiles, hardLinkFiles, snapshotPaths);
+ }
+
+ /**
+ * The snapshotInfo table may contain a snapshot that
+ * doesn't yet exist on the fs, so wait a few seconds for it.
+ * @param checkpoint Checkpoint containing snapshot entries expected.
+ * @return Set of expected snapshot dirs.
+ */
+ private Set<Path> waitForSnapshotDirs(DBCheckpoint checkpoint)
+ throws IOException {
+
+ OzoneConfiguration conf = getConf();
+
+ Set<Path> snapshotPaths = new HashSet<>();
+
+ // get snapshotInfo entries
+ OmMetadataManagerImpl checkpointMetadataManager =
+ OmMetadataManagerImpl.createCheckpointMetadataManager(
+ conf, checkpoint);
+ try (TableIterator<String, ? extends Table.KeyValue<String, SnapshotInfo>>
+ iterator = checkpointMetadataManager
+ .getSnapshotInfoTable().iterator()) {
+
+ // For each entry, wait for corresponding directory.
+ while (iterator.hasNext()) {
+ Table.KeyValue<String, SnapshotInfo> entry = iterator.next();
+ Path path = Paths.get(getSnapshotPath(conf, entry.getValue()));
+ waitForDirToExist(path);
+ snapshotPaths.add(path);
+ }
+ }
+ return snapshotPaths;
+ }
+
+ private void waitForDirToExist(Path dir) throws IOException {
+ if (!RDBCheckpointManager.waitForCheckpointDirectoryExist(dir.toFile())) {
Review Comment:
Optional: Should we move `waitForCheckpointDirectoryExist` to some Utils
class?
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java:
##########
@@ -82,4 +115,174 @@ public void init() throws ServletException {
allowedGroups,
om.isSpnegoEnabled());
}
+
+ @Override
+ public void writeDbDataToStream(DBCheckpoint checkpoint,
+ HttpServletRequest request,
+ OutputStream destination)
+ throws IOException, InterruptedException, CompressorException {
+ // Map of inodes to path.
+ Map<Object, Path> copyFiles = new HashMap<>();
+ // Map of link to path.
+ Map<Path, Path> hardLinkFiles = new HashMap<>();
+
+ getFilesForArchive(checkpoint, copyFiles, hardLinkFiles,
+ includeSnapshotData(request));
+
+ try (CompressorOutputStream gzippedOut = new CompressorStreamFactory()
+ .createCompressorOutputStream(
+ CompressorStreamFactory.GZIP, destination);
+ TarArchiveOutputStream archiveOutputStream =
+ new TarArchiveOutputStream(gzippedOut)
+ ) {
+ archiveOutputStream
+ .setLongFileMode(TarArchiveOutputStream.LONGFILE_POSIX);
+ writeFilesToArchive(copyFiles, hardLinkFiles, archiveOutputStream);
+ } catch (CompressorException e) {
+ throw new IOException(
+ "Can't compress the checkpoint: " +
+ checkpoint.getCheckpointLocation(), e);
+ }
+ }
+
+ private void getFilesForArchive(DBCheckpoint checkpoint,
+ Map<Object, Path> copyFiles,
+ Map<Path, Path> hardLinkFiles,
+ boolean includeSnapshotData)
+ throws IOException, InterruptedException {
+
+ // Get the active fs files.
+ Path dir = checkpoint.getCheckpointLocation();
+ processDir(dir, copyFiles, hardLinkFiles, new HashSet<>());
+
+ if (!includeSnapshotData) {
+ return;
+ }
+
+ // Get the snapshot files.
+ Set<Path> snapshotPaths = waitForSnapshotDirs(checkpoint);
+ Path snapshotDir = Paths.get(OMStorage.getOmDbDir(getConf()).toString(),
+ OM_SNAPSHOT_DIR);
+ processDir(snapshotDir, copyFiles, hardLinkFiles, snapshotPaths);
+ }
+
+ /**
+ * The snapshotInfo table may contain a snapshot that
+ * doesn't yet exist on the fs, so wait a few seconds for it.
+ * @param checkpoint Checkpoint containing snapshot entries expected.
+ * @return Set of expected snapshot dirs.
+ */
+ private Set<Path> waitForSnapshotDirs(DBCheckpoint checkpoint)
+ throws IOException, InterruptedException {
+
+ OzoneConfiguration conf = getConf();
+
+ Set<Path> snapshotPaths = new HashSet<>();
+
+ // get snapshotInfo entries
+ OmMetadataManagerImpl checkpointMetadataManager =
+ OmMetadataManagerImpl.createCheckpointMetadataManager(
+ conf, checkpoint);
+ try (TableIterator<String, ? extends Table.KeyValue<String, SnapshotInfo>>
+ iterator = checkpointMetadataManager
+ .getSnapshotInfoTable().iterator()) {
+
+ // For each entry, wait for corresponding directory.
+ while (iterator.hasNext()) {
+ Table.KeyValue<String, SnapshotInfo> entry = iterator.next();
+ Path path = Paths.get(getSnapshotPath(conf, entry.getValue()));
+ waitForDirToExist(path);
+ snapshotPaths.add(path);
+ }
+ }
+ return snapshotPaths;
+ }
+
+ private void waitForDirToExist(Path dir)
+ throws IOException, InterruptedException {
+ long endTime = System.currentTimeMillis() +
+ Duration.parse(DURATION_TO_WAIT_FOR_DIRECTORY).toMillis();
+ while (!dir.toFile().exists()) {
+ Thread.sleep(100);
+ if (System.currentTimeMillis() > endTime) {
+ throw new IOException("snapshot dir doesn't exist: " + dir);
+ }
+ }
+ }
+
+ @SuppressFBWarnings("NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE")
+ private void processDir(Path dir, Map<Object, Path> copyFiles,
+ Map<Path, Path> hardLinkFiles,
+ Set<Path> snapshotPaths)
+ throws IOException {
+ try (Stream<Path> files = Files.list(dir)) {
+ for (Path file : files.collect(Collectors.toList())) {
+ if (file.toFile().isDirectory()) {
+ // Skip any unexpected snapshot files.
+ if (file.getParent().toString().endsWith(OM_SNAPSHOT_CHECKPOINT_DIR)
+ && !snapshotPaths.contains(file)) {
+ continue;
+ }
+ processDir(file, copyFiles, hardLinkFiles, snapshotPaths);
+ } else {
+ processFile(file, copyFiles, hardLinkFiles);
+ }
+ }
+ }
+ }
+
+ private void processFile(Path file, Map<Object, Path> copyFiles,
+ Map<Path, Path> hardLinkFiles) throws IOException {
+ // Get the inode.
+ Object key = OmSnapshotManager.getINode(file);
+ // If we already have the inode, store as hard link.
+ if (copyFiles.containsKey(key)) {
+ hardLinkFiles.put(file, copyFiles.get(key));
+ } else {
+ copyFiles.put(key, file);
+ }
+ }
+
+ // Returns value of http request parameter.
+ private boolean includeSnapshotData(HttpServletRequest request) {
+ String includeParam =
Review Comment:
[Boolean.parseBoolean()](https://docs.oracle.com/javase/7/docs/api/java/lang/Boolean.html#parseBoolean(java.lang.String))
does a null check for you. Could be simply to:
```suggestion
String includeParam =
request.getParameter(OZONE_DB_CHECKPOINT_INCLUDE_SNAPSHOT_DATA);
return Boolean.parseBoolean(includeParam);
```
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RDBStore.java:
##########
@@ -134,7 +130,7 @@ public RDBStore(File dbFile, ManagedDBOptions dbOptions,
//create checkpoints directory if not exists.
checkpointsParentDir =
- Paths.get(dbLocation.getParent(), "db.checkpoints").toString();
+ Paths.get(dbLocation.getParent(), OM_CHECKPOINT_DIR).toString();
Review Comment:
```suggestion
dbLocation.getParent() + OM_KEY_PREFIX + OM_CHECKPOINT_DIR;
```
I feel it is unnecessary to get create Path and then get the String from it.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -260,4 +288,89 @@ private void verifySnapshotInfoForSnapDiff(final
SnapshotInfo fromSnapshot,
" should be older than to toSnapshot:" + toSnapshot.getName());
}
}
+ /**
+ * Create file of links to add to tarball.
+ * Format of entries are either:
+ * dir1/fileTo fileFrom
+ * for files in active db or:
+ * dir1/fileTo dir2/fileFrom
+ * for files in another directory, (either another snapshot dir or
+ * sst compaction backup directory)
+ * @param truncateLength - Length of initial path to trim in file path.
+ * @param hardLinkFiles - Map of link->file paths.
+ * @return Path to the file of links created.
+ */
+ @SuppressFBWarnings({"NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE"})
+ static Path createHardLinkList(int truncateLength,
Review Comment:
Both `createHardLinkList(int truncateLength, Map<Path, Path> hardLinkFiles)`
and `createHardLinks(Path dbPath)` don't belong to `OmSnapshotManager`
according to me.
As I said for `getINode()`, these functions belong to separate Util/Helper
class.
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java:
##########
@@ -562,7 +564,18 @@ private OzoneConsts() {
public static final int OZONE_MAXIMUM_ACCESS_ID_LENGTH = 100;
public static final String OM_SNAPSHOT_NAME = "snapshotName";
+ public static final String OM_CHECKPOINT_DIR = "db.checkpoints";
public static final String OM_SNAPSHOT_DIR = "db.snapshots";
+ public static final String OM_SNAPSHOT_CHECKPOINT_DIR = OM_SNAPSHOT_DIR
+ + OM_KEY_PREFIX + "checkpointState";
+ public static final String OM_SNAPSHOT_DIFF_DIR = OM_SNAPSHOT_DIR
+ + OM_KEY_PREFIX + "diffState";
+
+ // Name of the SST file backup directory placed under diff dir.
Review Comment:
Should it be `// Name of the SST file backup directory placed under metadata
dir.` or `// Name of the SST file backup directory placed under snapshot dir.`?
Or may be `snapshot diff`?
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -162,6 +177,12 @@ public static DBCheckpoint createOmSnapshotCheckpoint(
return dbCheckpoint;
}
+ @VisibleForTesting
+ static Object getINode(Path file) throws IOException {
Review Comment:
This is unnecessary one liner function to me. What's the reason of putting
it here? It is not used anywhere inside `OmSnapshotManager`. If this has to be
a helper function, may be put it in some util class.
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java:
##########
@@ -562,7 +564,18 @@ private OzoneConsts() {
public static final int OZONE_MAXIMUM_ACCESS_ID_LENGTH = 100;
public static final String OM_SNAPSHOT_NAME = "snapshotName";
+ public static final String OM_CHECKPOINT_DIR = "db.checkpoints";
public static final String OM_SNAPSHOT_DIR = "db.snapshots";
+ public static final String OM_SNAPSHOT_CHECKPOINT_DIR = OM_SNAPSHOT_DIR
+ + OM_KEY_PREFIX + "checkpointState";
+ public static final String OM_SNAPSHOT_DIFF_DIR = OM_SNAPSHOT_DIR
+ + OM_KEY_PREFIX + "diffState";
+
+ // Name of the SST file backup directory placed under diff dir.
+ public static final String OM_COMPACTION_BACKUP_DIR =
"compaction-sst-backup";
Review Comment:
```suggestion
public static final String OM_COMPACTION_SST_BACKUP_DIR =
"compaction-sst-backup";
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java:
##########
@@ -82,4 +115,174 @@ public void init() throws ServletException {
allowedGroups,
om.isSpnegoEnabled());
}
+
+ @Override
+ public void writeDbDataToStream(DBCheckpoint checkpoint,
+ HttpServletRequest request,
+ OutputStream destination)
+ throws IOException, InterruptedException, CompressorException {
Review Comment:
Are we still throwing `CompressorException` somewhere? I see we have a catch
for it at line # 141.
--
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]
