hemantk-12 commented on code in PR #8245:
URL: https://github.com/apache/ozone/pull/8245#discussion_r2054929809
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java:
##########
@@ -302,36 +304,59 @@ private boolean getFilesForArchive(DBCheckpoint
checkpoint,
logEstimatedTarballSize(checkpoint, includeSnapshotData);
}
- // Get the active fs files.
- Path dir = checkpoint.getCheckpointLocation();
- if (!processDir(dir, copyFiles, hardLinkFiles, sstFilesToExclude,
- new HashSet<>(), excluded, copySize, null)) {
- return false;
- }
- if (!includeSnapshotData) {
- return true;
- }
+ if (includeSnapshotData) {
+ // Get the snapshot files.
+ Set<Path> snapshotPaths = getSnapshotDirs(checkpoint, true);
+ Path snapshotDir = getSnapshotDir();
+ boolean processedSnapshotData = processDir(snapshotDir, copyFiles,
hardLinkFiles,
+ sstFilesToExclude, snapshotPaths, excluded, copySize, null);
+ if (LOG.isDebugEnabled()) {
+ if (processedSnapshotData) {
+ LOG.debug("Finished processing OM snapshot data");
+ }
+ }
Review Comment:
nit: there is no need to do `LOG.isDebugEnabled()` check for a simple
string. It is useful when String concatenation is needed.
```suggestion
if (processedSnapshotData) {
LOG.debug("Finished processing OM snapshot data");
}
```
Alternatively
```
if (LOG.isDebugEnabled() && processedSnapshotData) {
LOG.debug("Finished processing OM snapshot data");
}
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java:
##########
@@ -302,36 +304,59 @@ private boolean getFilesForArchive(DBCheckpoint
checkpoint,
logEstimatedTarballSize(checkpoint, includeSnapshotData);
}
- // Get the active fs files.
- Path dir = checkpoint.getCheckpointLocation();
- if (!processDir(dir, copyFiles, hardLinkFiles, sstFilesToExclude,
- new HashSet<>(), excluded, copySize, null)) {
- return false;
- }
Review Comment:
nit: remove this extra line.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java:
##########
@@ -302,36 +304,59 @@ private boolean getFilesForArchive(DBCheckpoint
checkpoint,
logEstimatedTarballSize(checkpoint, includeSnapshotData);
}
- // Get the active fs files.
- Path dir = checkpoint.getCheckpointLocation();
- if (!processDir(dir, copyFiles, hardLinkFiles, sstFilesToExclude,
- new HashSet<>(), excluded, copySize, null)) {
- return false;
- }
- if (!includeSnapshotData) {
- return true;
- }
+ if (includeSnapshotData) {
+ // Get the snapshot files.
+ Set<Path> snapshotPaths = getSnapshotDirs(checkpoint, true);
+ Path snapshotDir = getSnapshotDir();
+ boolean processedSnapshotData = processDir(snapshotDir, copyFiles,
hardLinkFiles,
+ sstFilesToExclude, snapshotPaths, excluded, copySize, null);
+ if (LOG.isDebugEnabled()) {
+ if (processedSnapshotData) {
+ LOG.debug("Finished processing OM snapshot data");
+ }
+ }
+ if (!processedSnapshotData) {
+ return false;
+ }
- // Get the snapshot files.
- Set<Path> snapshotPaths = getSnapshotDirs(checkpoint, true);
- Path snapshotDir = getSnapshotDir();
- if (!processDir(snapshotDir, copyFiles, hardLinkFiles, sstFilesToExclude,
- snapshotPaths, excluded, copySize, null)) {
- return false;
- }
- // Process the tmp sst compaction dir.
- if (!processDir(sstBackupDir.getTmpDir().toPath(), copyFiles,
hardLinkFiles,
- sstFilesToExclude, new HashSet<>(), excluded, copySize,
- sstBackupDir.getOriginalDir().toPath())) {
- return false;
+ // Process the tmp sst compaction dir.
+ boolean processedTmpSstCompactionDir =
processDir(sstBackupDir.getTmpDir().toPath(),
+ copyFiles, hardLinkFiles, sstFilesToExclude, new HashSet<>(),
+ excluded, copySize, sstBackupDir.getOriginalDir().toPath());
+ if (LOG.isDebugEnabled()) {
+ if (processedTmpSstCompactionDir) {
+ LOG.debug("Finished processing tmp SST Compaction Dir");
+ }
+ }
+ if (!processedTmpSstCompactionDir) {
+ return false;
+ }
+
+ // Process the tmp compaction log dir.
+ boolean processedTmpCompactionLogDir =
processDir(compactionLogDir.getTmpDir().toPath(),
+ copyFiles, hardLinkFiles, sstFilesToExclude, new HashSet<>(),
+ excluded, copySize, compactionLogDir.getOriginalDir().toPath());
Review Comment:
nit: let's just call it `processedCompactionLogDir`. We create tmp for local
referencing only.
```suggestion
boolean processedCompactionLogDir =
processDir(compactionLogDir.getTmpDir().toPath(),
copyFiles, hardLinkFiles, sstFilesToExclude, new HashSet<>(),
excluded, copySize,
compactionLogDir.getOriginalDir().toPath());
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java:
##########
@@ -302,36 +304,59 @@ private boolean getFilesForArchive(DBCheckpoint
checkpoint,
logEstimatedTarballSize(checkpoint, includeSnapshotData);
}
- // Get the active fs files.
- Path dir = checkpoint.getCheckpointLocation();
- if (!processDir(dir, copyFiles, hardLinkFiles, sstFilesToExclude,
- new HashSet<>(), excluded, copySize, null)) {
- return false;
- }
- if (!includeSnapshotData) {
- return true;
- }
+ if (includeSnapshotData) {
+ // Get the snapshot files.
+ Set<Path> snapshotPaths = getSnapshotDirs(checkpoint, true);
+ Path snapshotDir = getSnapshotDir();
+ boolean processedSnapshotData = processDir(snapshotDir, copyFiles,
hardLinkFiles,
+ sstFilesToExclude, snapshotPaths, excluded, copySize, null);
+ if (LOG.isDebugEnabled()) {
+ if (processedSnapshotData) {
+ LOG.debug("Finished processing OM snapshot data");
+ }
+ }
+ if (!processedSnapshotData) {
+ return false;
+ }
- // Get the snapshot files.
- Set<Path> snapshotPaths = getSnapshotDirs(checkpoint, true);
- Path snapshotDir = getSnapshotDir();
- if (!processDir(snapshotDir, copyFiles, hardLinkFiles, sstFilesToExclude,
- snapshotPaths, excluded, copySize, null)) {
- return false;
- }
- // Process the tmp sst compaction dir.
- if (!processDir(sstBackupDir.getTmpDir().toPath(), copyFiles,
hardLinkFiles,
- sstFilesToExclude, new HashSet<>(), excluded, copySize,
- sstBackupDir.getOriginalDir().toPath())) {
- return false;
+ // Process the tmp sst compaction dir.
+ boolean processedTmpSstCompactionDir =
processDir(sstBackupDir.getTmpDir().toPath(),
+ copyFiles, hardLinkFiles, sstFilesToExclude, new HashSet<>(),
+ excluded, copySize, sstBackupDir.getOriginalDir().toPath());
Review Comment:
```suggestion
boolean processedSstCompactionBackupDir =
processDir(sstBackupDir.getTmpDir().toPath(),
copyFiles, hardLinkFiles, sstFilesToExclude, new HashSet<>(),
excluded, copySize, sstBackupDir.getOriginalDir().toPath());
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java:
##########
@@ -380,18 +408,24 @@ private Set<Path> getSnapshotDirs(DBCheckpoint
checkpoint, boolean waitForDir)
iterator = checkpointMetadataManager
.getSnapshotInfoTable().iterator()) {
- // For each entry, wait for corresponding directory.
+ // For each entry, wait for corresponding directory if the key is
Review Comment:
Are we assuming that whatever is flushed to disk must have a snapshot
directory?
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java:
##########
@@ -302,36 +304,59 @@ private boolean getFilesForArchive(DBCheckpoint
checkpoint,
logEstimatedTarballSize(checkpoint, includeSnapshotData);
}
- // Get the active fs files.
- Path dir = checkpoint.getCheckpointLocation();
- if (!processDir(dir, copyFiles, hardLinkFiles, sstFilesToExclude,
- new HashSet<>(), excluded, copySize, null)) {
- return false;
- }
- if (!includeSnapshotData) {
- return true;
- }
+ if (includeSnapshotData) {
+ // Get the snapshot files.
+ Set<Path> snapshotPaths = getSnapshotDirs(checkpoint, true);
+ Path snapshotDir = getSnapshotDir();
+ boolean processedSnapshotData = processDir(snapshotDir, copyFiles,
hardLinkFiles,
+ sstFilesToExclude, snapshotPaths, excluded, copySize, null);
+ if (LOG.isDebugEnabled()) {
+ if (processedSnapshotData) {
+ LOG.debug("Finished processing OM snapshot data");
+ }
+ }
+ if (!processedSnapshotData) {
+ return false;
+ }
- // Get the snapshot files.
- Set<Path> snapshotPaths = getSnapshotDirs(checkpoint, true);
- Path snapshotDir = getSnapshotDir();
- if (!processDir(snapshotDir, copyFiles, hardLinkFiles, sstFilesToExclude,
- snapshotPaths, excluded, copySize, null)) {
- return false;
- }
- // Process the tmp sst compaction dir.
- if (!processDir(sstBackupDir.getTmpDir().toPath(), copyFiles,
hardLinkFiles,
- sstFilesToExclude, new HashSet<>(), excluded, copySize,
- sstBackupDir.getOriginalDir().toPath())) {
- return false;
+ // Process the tmp sst compaction dir.
+ boolean processedTmpSstCompactionDir =
processDir(sstBackupDir.getTmpDir().toPath(),
+ copyFiles, hardLinkFiles, sstFilesToExclude, new HashSet<>(),
+ excluded, copySize, sstBackupDir.getOriginalDir().toPath());
+ if (LOG.isDebugEnabled()) {
Review Comment:
Same as previous.
--
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]
