hemantk-12 commented on code in PR #5215:
URL: https://github.com/apache/ozone/pull/5215#discussion_r1310931911
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java:
##########
@@ -159,11 +159,18 @@ public void writeDbDataToStream(DBCheckpoint checkpoint,
archiveOutputStream
.setBigNumberMode(TarArchiveOutputStream.BIGNUMBER_POSIX);
// Files to be excluded from tarball
Review Comment:
nit: Either remove this comment or move it before line number 169.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java:
##########
@@ -487,13 +507,40 @@ public static long processFile(Path file, Map<Path, Path>
copyFiles,
return fileSize;
}
- // If fileName exists in "files" parameter,
- // it should be linked to path in files.
- private static Path findLinkPath(Collection<Path> files, String fileName) {
- for (Path p: files) {
- Path file = p.getFileName();
- if ((file != null) && file.toString().equals(fileName)) {
- return p;
+ /**
+ * Find a valid hard link path for file. If fileName exists in
+ * "files" parameter, file should be linked to the corresponding
+ * dest path in files.
+ * @param files - Map of src/dest path to files available for
+ * linking.
+ * @param file - File to be linked.
+ * @return dest path of file to be linked to.
+ */
+ private static Path findLinkPath(Map<Path, Path> files, Path file)
+ throws IOException {
+ // findbugs nonsense
+ Path fileNamePath = file.getFileName();
+ if (fileNamePath == null) {
+ throw new IOException("file has no filename:" + file);
+ }
+ String fileName = fileNamePath.toString();
+
+ for (Map.Entry<Path, Path> entry: files.entrySet()) {
+ Path srcPath = entry.getKey();
+ Path destPath = entry.getValue();
+ if (srcPath.toString().endsWith(fileName)) {
Review Comment:
nit: nesting can be remove either:
```
if (!srcPath.toString().endsWith(fileName)) {
continue;
}
if (!srcPath.toFile().exists()) {
continue;
}
if (Objects.equals(OmSnapshotUtils.getINode(srcPath),
OmSnapshotUtils.getINode(file))) {
return destPath;
} else {
LOG.info("Found non linked sst files with the same name: {}, {}",
srcPath, file);
}
```
Or
```
if (srcPath.toString().endsWith(fileName) && srcPath.toFile().exists()
&& Objects.equals(OmSnapshotUtils.getINode(srcPath),
OmSnapshotUtils.getINode(file))) {
return destPath;
} else {
LOG.info("Found non linked sst files with the same name: {}, {}",
srcPath, file);
}
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java:
##########
@@ -159,11 +159,18 @@ public void writeDbDataToStream(DBCheckpoint checkpoint,
archiveOutputStream
.setBigNumberMode(TarArchiveOutputStream.BIGNUMBER_POSIX);
// Files to be excluded from tarball
- Set<Path> toExcludeFiles = normalizeExcludeList(toExcludeList,
- checkpoint.getCheckpointLocation());
+ RocksDBCheckpointDiffer differ =
+ getDbStore().getRocksDBCheckpointDiffer();
+ DirectoryData sstBackupDir = new DirectoryData(tmpdir,
+ differ.getSSTBackupDir());
+ DirectoryData compactionLogDir = new DirectoryData(tmpdir,
+ differ.getCompactionLogDir());
+
+ Map<Path, Path> toExcludeFiles = normalizeExcludeList(toExcludeList,
Review Comment:
Is `toExcludeList` only applicable to sstFiles? or also to
compactionLogFiles?
nit: if it is for sstFiles only, may be variable name can be changed
`toExcludeSstFileList` to emphasize on SST part.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java:
##########
@@ -172,20 +179,41 @@ hardLinkFiles, toExcludeFiles,
includeSnapshotData(request),
}
}
- // Format list from follower to match data on leader.
+ /**
+ * Format the list of excluded sst files from follower to match data
+ * on leader.
+ * @param toExcludeList - list of excluded sst files from follower
+ * @param checkpointLocation - location of checkpoint for this tarball
+ * @param sstBackupDir - location info about sstBackupDir
+ * @return A Map of src and dest paths for the entries in the toExcludeList.
+ * Formatted in a manner analogous to the copyFiles data structure
+ * described above. Because this structure only points to sst files,
+ * the implementation ignores the compactionLog dir, (which doesn't
+ * include sst files.)
+ */
@VisibleForTesting
- public static Set<Path> normalizeExcludeList(List<String> toExcludeList,
- Path checkpointLocation) {
- Set<Path> paths = new HashSet<>();
+ public static Map<Path, Path> normalizeExcludeList(
+ List<String> toExcludeList,
+ Path checkpointLocation,
+ DirectoryData sstBackupDir) {
+ Map<Path, Path> paths = new HashMap<>();
for (String s : toExcludeList) {
- if (!s.startsWith(OM_SNAPSHOT_DIR)) {
+ Path metaDirPath = getMetaDirPath(checkpointLocation);
+ Path destPath = Paths.get(metaDirPath.toString(), s);
Review Comment:
QQ: Is it possible that `destPath` doesn't exist? What happens if `destPath`
doesn't exist? Do we fail the request? Should we add a check for it?
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java:
##########
@@ -172,20 +179,41 @@ hardLinkFiles, toExcludeFiles,
includeSnapshotData(request),
}
}
- // Format list from follower to match data on leader.
+ /**
+ * Format the list of excluded sst files from follower to match data
+ * on leader.
+ * @param toExcludeList - list of excluded sst files from follower
+ * @param checkpointLocation - location of checkpoint for this tarball
+ * @param sstBackupDir - location info about sstBackupDir
+ * @return A Map of src and dest paths for the entries in the toExcludeList.
+ * Formatted in a manner analogous to the copyFiles data structure
+ * described above. Because this structure only points to sst files,
+ * the implementation ignores the compactionLog dir, (which doesn't
+ * include sst files.)
+ */
@VisibleForTesting
- public static Set<Path> normalizeExcludeList(List<String> toExcludeList,
- Path checkpointLocation) {
- Set<Path> paths = new HashSet<>();
+ public static Map<Path, Path> normalizeExcludeList(
+ List<String> toExcludeList,
+ Path checkpointLocation,
+ DirectoryData sstBackupDir) {
+ Map<Path, Path> paths = new HashMap<>();
for (String s : toExcludeList) {
- if (!s.startsWith(OM_SNAPSHOT_DIR)) {
+ Path metaDirPath = getMetaDirPath(checkpointLocation);
Review Comment:
nit: `metaDirPath` can be defined before the loop.
--
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]
