Copilot commented on code in PR #9131:
URL: https://github.com/apache/ozone/pull/9131#discussion_r2426801889
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServletInodeBasedXfer.java:
##########
@@ -250,6 +250,17 @@ public void writeDbDataToStream(HttpServletRequest
request, OutputStream destina
writeDBToArchive(sstFilesToExclude, checkpointDir,
maxTotalSstSize, archiveOutputStream, tmpdir, hardLinkFileMap,
false);
if (includeSnapshotData) {
+ // get the list of snapshots from the checkpoint
+ OmMetadataManagerImpl checkpointMetadataManager = null;
+ try {
+ checkpointMetadataManager =
+
OmMetadataManagerImpl.createCheckpointMetadataManager(om.getConfiguration(),
checkpoint);
+ snapshotPaths = getSnapshotDirs(checkpointMetadataManager);
+ } finally {
+ if (checkpointMetadataManager != null) {
+ checkpointMetadataManager.stop();
+ }
Review Comment:
Consider using try-with-resources pattern instead of manual resource
management. If OmMetadataManagerImpl implements AutoCloseable, this would
ensure proper cleanup even if an exception occurs during getSnapshotDirs().
```suggestion
try (OmMetadataManagerImpl checkpointMetadataManager =
OmMetadataManagerImpl.createCheckpointMetadataManager(om.getConfiguration(),
checkpoint)) {
snapshotPaths = getSnapshotDirs(checkpointMetadataManager);
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServletInodeBasedXfer.java:
##########
@@ -284,7 +295,7 @@ public void writeDbDataToStream(HttpServletRequest request,
OutputStream destina
* @param hardLinkFileMap Map of hardlink file paths to their unique
identifiers for deduplication.
* @throws IOException if an I/O error occurs during processing.
*/
- private void transferSnapshotData(Set<String> sstFilesToExclude, Path
tmpdir, Set<Path> snapshotPaths,
+ void transferSnapshotData(Set<String> sstFilesToExclude, Path tmpdir,
Set<Path> snapshotPaths,
Review Comment:
Changing method visibility from private to package-private breaks
encapsulation. Consider creating a separate testable method or using reflection
in tests to avoid exposing internal implementation details.
```suggestion
private void transferSnapshotData(Set<String> sstFilesToExclude, Path
tmpdir, Set<Path> snapshotPaths,
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServletInodeBasedXfer.java:
##########
@@ -476,7 +487,7 @@ private boolean writeDBToArchive(Set<String>
sstFilesToExclude, Path dbDir, Atom
* @return The created database checkpoint.
* @throws IOException If an error occurs during checkpoint creation or file
copying.
*/
- private DBCheckpoint createAndPrepareCheckpoint(Path tmpdir, boolean flush)
throws IOException {
+ DBCheckpoint createAndPrepareCheckpoint(Path tmpdir, boolean flush) throws
IOException {
Review Comment:
Changing method visibility from private to package-private breaks
encapsulation. Consider creating a separate testable method or using reflection
in tests to avoid exposing internal implementation details.
```suggestion
private DBCheckpoint createAndPrepareCheckpoint(Path tmpdir, boolean
flush) throws IOException {
```
--
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]
