Copilot commented on code in PR #9129:
URL: https://github.com/apache/ozone/pull/9129#discussion_r2426801021
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServletInodeBasedXfer.java:
##########
@@ -243,22 +245,26 @@ public void writeDbDataToStream(HttpServletRequest
request, OutputStream destina
if (shouldContinue) {
// we finished transferring files from snapshot DB's by now and
// this is the last step where we transfer the active om.db contents
- checkpoint = createAndPrepareCheckpoint(tmpdir, true);
- // unlimited files as we want the Active DB contents to be transferred
in a single batch
- maxTotalSstSize.set(Long.MAX_VALUE);
- Path checkpointDir = checkpoint.getCheckpointLocation();
- writeDBToArchive(sstFilesToExclude, checkpointDir,
- maxTotalSstSize, archiveOutputStream, tmpdir, hardLinkFileMap,
false);
- if (includeSnapshotData) {
- Path tmpCompactionLogDir =
tmpdir.resolve(getCompactionLogDir().getFileName());
- Path tmpSstBackupDir =
tmpdir.resolve(getSstBackupDir().getFileName());
- writeDBToArchive(sstFilesToExclude, tmpCompactionLogDir,
maxTotalSstSize, archiveOutputStream, tmpdir,
- hardLinkFileMap, getCompactionLogDir(), false);
- writeDBToArchive(sstFilesToExclude, tmpSstBackupDir,
maxTotalSstSize, archiveOutputStream, tmpdir,
- hardLinkFileMap, getSstBackupDir(), false);
- // This is done to ensure all data to be copied correctly is flushed
in the snapshot DB
- transferSnapshotData(sstFilesToExclude, tmpdir, snapshotPaths,
maxTotalSstSize,
- archiveOutputStream, hardLinkFileMap);
+ SnapshotCache snapshotCache =
om.getOmSnapshotManager().getSnapshotCache();
+ // lock is null when includeSnapshotData=false, no snapshot
consistency needed
+ try (UncheckedAutoCloseableSupplier<OMLockDetails> lock =
includeSnapshotData ? snapshotCache.lock() : null) {
Review Comment:
Using try-with-resources with a potentially null resource is error-prone.
Consider restructuring to avoid null in try-with-resources or use conditional
locking outside the try block.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServletInodeBasedXfer.java:
##########
@@ -243,22 +245,26 @@ public void writeDbDataToStream(HttpServletRequest
request, OutputStream destina
if (shouldContinue) {
// we finished transferring files from snapshot DB's by now and
// this is the last step where we transfer the active om.db contents
- checkpoint = createAndPrepareCheckpoint(tmpdir, true);
- // unlimited files as we want the Active DB contents to be transferred
in a single batch
- maxTotalSstSize.set(Long.MAX_VALUE);
- Path checkpointDir = checkpoint.getCheckpointLocation();
- writeDBToArchive(sstFilesToExclude, checkpointDir,
- maxTotalSstSize, archiveOutputStream, tmpdir, hardLinkFileMap,
false);
- if (includeSnapshotData) {
- Path tmpCompactionLogDir =
tmpdir.resolve(getCompactionLogDir().getFileName());
- Path tmpSstBackupDir =
tmpdir.resolve(getSstBackupDir().getFileName());
- writeDBToArchive(sstFilesToExclude, tmpCompactionLogDir,
maxTotalSstSize, archiveOutputStream, tmpdir,
- hardLinkFileMap, getCompactionLogDir(), false);
- writeDBToArchive(sstFilesToExclude, tmpSstBackupDir,
maxTotalSstSize, archiveOutputStream, tmpdir,
- hardLinkFileMap, getSstBackupDir(), false);
- // This is done to ensure all data to be copied correctly is flushed
in the snapshot DB
- transferSnapshotData(sstFilesToExclude, tmpdir, snapshotPaths,
maxTotalSstSize,
- archiveOutputStream, hardLinkFileMap);
+ SnapshotCache snapshotCache =
om.getOmSnapshotManager().getSnapshotCache();
+ // lock is null when includeSnapshotData=false, no snapshot
consistency needed
Review Comment:
The comment should clarify what happens when lock is null in the
try-with-resources block, as this pattern can be confusing to maintainers.
```suggestion
/*
* When includeSnapshotData is false, lock is set to null and no
locking is performed.
* In this case, the try-with-resources block does not call close()
on any resource,
* which is intentional because snapshot consistency is not required.
* This pattern is safe in Java: try-with-resources will simply skip
closing if the resource is null.
*/
```
--
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]
