Copilot commented on code in PR #10774:
URL: https://github.com/apache/ozone/pull/10774#discussion_r3718284082
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/defrag/SnapshotDefragService.java:
##########
@@ -573,20 +574,73 @@ OmMetadataManagerImpl createCheckpoint(SnapshotInfo
snapshotInfo,
Set<String> incrementalColumnFamilies) throws IOException {
try (UncheckedAutoCloseableSupplier<OmSnapshot> snapshot =
omSnapshotManager.getActiveSnapshot(
snapshotInfo.getVolumeName(), snapshotInfo.getBucketName(),
snapshotInfo.getName())) {
- DBCheckpoint checkpoint =
snapshot.get().getMetadataManager().getStore().getCheckpoint(tmpDefragDir,
true);
- try (OmMetadataManagerImpl metadataManagerBeforeTruncate =
- createDefragCheckpointMetadataManager(checkpoint, false)) {
- DBStore dbStore = metadataManagerBeforeTruncate.getStore();
- for (String table : metadataManagerBeforeTruncate.listTableNames()) {
- if (!incrementalColumnFamilies.contains(table)) {
- dbStore.dropTable(table);
+ DBStore snapshotStore = snapshot.get().getMetadataManager().getStore();
+ DBCheckpoint checkpoint = snapshotStore.getCheckpoint(tmpDefragDir,
true);
+ if (checkpoint == null) {
+ deletePartialCheckpointDirs(snapshotStore.getDbLocation().getName());
+ throw new IOException("Failed to create checkpoint under " +
tmpDefragDir + " for snapshot: "
+ + snapshotInfo.getTableKey() + " (ID: " +
snapshotInfo.getSnapshotId() + ")");
+ }
+ Path checkpointLocation = checkpoint.getCheckpointLocation();
+ boolean checkpointSuccessful = false;
+ try {
+ try (OmMetadataManagerImpl metadataManagerBeforeTruncate =
+ createDefragCheckpointMetadataManager(checkpoint, false)) {
+ DBStore dbStore = metadataManagerBeforeTruncate.getStore();
+ for (String table : metadataManagerBeforeTruncate.listTableNames()) {
+ if (!incrementalColumnFamilies.contains(table)) {
+ dbStore.dropTable(table);
+ }
+ }
+ } catch (Exception e) {
+ throw new IOException("Failed to close checkpoint of snapshot: " +
snapshotInfo.getSnapshotId(), e);
+ }
Review Comment:
This catch wraps *any* exception from truncation or closing, but the error
message says "Failed to close checkpoint". That can mislead debugging when the
failure is actually during `dropTable(...)` or opening the checkpoint metadata
manager.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/defrag/SnapshotDefragService.java:
##########
@@ -573,20 +574,73 @@ OmMetadataManagerImpl createCheckpoint(SnapshotInfo
snapshotInfo,
Set<String> incrementalColumnFamilies) throws IOException {
try (UncheckedAutoCloseableSupplier<OmSnapshot> snapshot =
omSnapshotManager.getActiveSnapshot(
snapshotInfo.getVolumeName(), snapshotInfo.getBucketName(),
snapshotInfo.getName())) {
- DBCheckpoint checkpoint =
snapshot.get().getMetadataManager().getStore().getCheckpoint(tmpDefragDir,
true);
- try (OmMetadataManagerImpl metadataManagerBeforeTruncate =
- createDefragCheckpointMetadataManager(checkpoint, false)) {
- DBStore dbStore = metadataManagerBeforeTruncate.getStore();
- for (String table : metadataManagerBeforeTruncate.listTableNames()) {
- if (!incrementalColumnFamilies.contains(table)) {
- dbStore.dropTable(table);
+ DBStore snapshotStore = snapshot.get().getMetadataManager().getStore();
+ DBCheckpoint checkpoint = snapshotStore.getCheckpoint(tmpDefragDir,
true);
+ if (checkpoint == null) {
+ deletePartialCheckpointDirs(snapshotStore.getDbLocation().getName());
+ throw new IOException("Failed to create checkpoint under " +
tmpDefragDir + " for snapshot: "
+ + snapshotInfo.getTableKey() + " (ID: " +
snapshotInfo.getSnapshotId() + ")");
+ }
+ Path checkpointLocation = checkpoint.getCheckpointLocation();
+ boolean checkpointSuccessful = false;
+ try {
+ try (OmMetadataManagerImpl metadataManagerBeforeTruncate =
+ createDefragCheckpointMetadataManager(checkpoint, false)) {
+ DBStore dbStore = metadataManagerBeforeTruncate.getStore();
+ for (String table : metadataManagerBeforeTruncate.listTableNames()) {
+ if (!incrementalColumnFamilies.contains(table)) {
+ dbStore.dropTable(table);
+ }
+ }
+ } catch (Exception e) {
+ throw new IOException("Failed to close checkpoint of snapshot: " +
snapshotInfo.getSnapshotId(), e);
+ }
+ // This will recreate the column families in the checkpoint.
+ OmMetadataManagerImpl result =
createDefragCheckpointMetadataManager(checkpoint, false);
+ checkpointSuccessful = true;
+ return result;
+ } finally {
+ if (!checkpointSuccessful && Files.exists(checkpointLocation)) {
+ try {
+ deleteDirectory(checkpointLocation);
+ } catch (IOException cleanupException) {
+ LOG.error("Failed to clean up checkpoint directory {} for
snapshot: {} (ID: {}). " +
+ "Disk space may not be freed. Manual cleanup may be required.",
+ checkpointLocation, snapshotInfo.getTableKey(),
snapshotInfo.getSnapshotId(),
+ cleanupException);
}
Review Comment:
PR description says cleanup failures are tracked via
`incNumSnapshotDefragFails()`, but this checkpoint-directory cleanup failure
path only logs. If you want ops visibility into cleanup failures (disk space
not freed), increment the metric here as well (or adjust the PR description).
This issue also appears on line 803 of the same file.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/defrag/SnapshotDefragService.java:
##########
@@ -573,20 +574,73 @@ OmMetadataManagerImpl createCheckpoint(SnapshotInfo
snapshotInfo,
Set<String> incrementalColumnFamilies) throws IOException {
try (UncheckedAutoCloseableSupplier<OmSnapshot> snapshot =
omSnapshotManager.getActiveSnapshot(
snapshotInfo.getVolumeName(), snapshotInfo.getBucketName(),
snapshotInfo.getName())) {
- DBCheckpoint checkpoint =
snapshot.get().getMetadataManager().getStore().getCheckpoint(tmpDefragDir,
true);
- try (OmMetadataManagerImpl metadataManagerBeforeTruncate =
- createDefragCheckpointMetadataManager(checkpoint, false)) {
- DBStore dbStore = metadataManagerBeforeTruncate.getStore();
- for (String table : metadataManagerBeforeTruncate.listTableNames()) {
- if (!incrementalColumnFamilies.contains(table)) {
- dbStore.dropTable(table);
+ DBStore snapshotStore = snapshot.get().getMetadataManager().getStore();
+ DBCheckpoint checkpoint = snapshotStore.getCheckpoint(tmpDefragDir,
true);
+ if (checkpoint == null) {
+ deletePartialCheckpointDirs(snapshotStore.getDbLocation().getName());
+ throw new IOException("Failed to create checkpoint under " +
tmpDefragDir + " for snapshot: "
+ + snapshotInfo.getTableKey() + " (ID: " +
snapshotInfo.getSnapshotId() + ")");
+ }
+ Path checkpointLocation = checkpoint.getCheckpointLocation();
+ boolean checkpointSuccessful = false;
+ try {
+ try (OmMetadataManagerImpl metadataManagerBeforeTruncate =
+ createDefragCheckpointMetadataManager(checkpoint, false)) {
+ DBStore dbStore = metadataManagerBeforeTruncate.getStore();
+ for (String table : metadataManagerBeforeTruncate.listTableNames()) {
+ if (!incrementalColumnFamilies.contains(table)) {
+ dbStore.dropTable(table);
+ }
+ }
+ } catch (Exception e) {
+ throw new IOException("Failed to close checkpoint of snapshot: " +
snapshotInfo.getSnapshotId(), e);
+ }
+ // This will recreate the column families in the checkpoint.
+ OmMetadataManagerImpl result =
createDefragCheckpointMetadataManager(checkpoint, false);
+ checkpointSuccessful = true;
+ return result;
+ } finally {
+ if (!checkpointSuccessful && Files.exists(checkpointLocation)) {
+ try {
+ deleteDirectory(checkpointLocation);
+ } catch (IOException cleanupException) {
+ LOG.error("Failed to clean up checkpoint directory {} for
snapshot: {} (ID: {}). " +
+ "Disk space may not be freed. Manual cleanup may be required.",
+ checkpointLocation, snapshotInfo.getTableKey(),
snapshotInfo.getSnapshotId(),
+ cleanupException);
}
}
- } catch (Exception e) {
- throw new IOException("Failed to close checkpoint of snapshot: " +
snapshotInfo.getSnapshotId(), e);
}
- // This will recreate the column families in the checkpoint.
- return createDefragCheckpointMetadataManager(checkpoint, false);
+ }
+ }
+
+ /**
+ * Deletes checkpoint directories left behind under {@link #tmpDefragDir}
for the given source
+ * RocksDB name after {@link DBStore#getCheckpoint} failed to produce a
checkpoint. The checkpoint
+ * directory name is only known to
+ * {@link org.apache.hadoop.hdds.utils.db.RDBCheckpointManager}, so any
leftover is located by its
+ * {@code <dbName>_} + {@code RDB_CHECKPOINT_PREFIX} naming convention
rather than by an exact path.
+ */
Review Comment:
Javadoc refers to `RDB_CHECKPOINT_PREFIX`, but the code (and
`RDBCheckpointManager`) uses `RDB_CHECKPOINT_DIR_PREFIX`. This makes the
cleanup naming convention unclear/misleading for future maintainers.
--
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]
