devmadhuu commented on code in PR #10950: URL: https://github.com/apache/ozone/pull/10950#discussion_r3714592779
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/tasks/ReconTaskControllerImpl.java:
##########
@@ -924,29 +927,32 @@ private void cleanupPreExistingCheckpoints() {
}
/**
- * Cleanup checkpoint files for a checkpointed OM metadata manager.
- * This method only removes the temporary checkpoint files without closing
database connections.
- * Used when the manager is closed via try-with-resources.
- *
- * @param checkpointedManager the checkpointed OM metadata manager
+ * Cleanup checkpointed OM metadata manager and associated checkpoint files.
+ * This method closes the database connections and removes the temporary
checkpoint files.
+ *
+ * @param checkpointedManager the checkpointed OM metadata manager to clean
up
*/
- private void cleanupCheckpointFiles(ReconOMMetadataManager
checkpointedManager) {
+ private void cleanupCheckpoint(ReconOMMetadataManager checkpointedManager) {
if (checkpointedManager == null) {
return;
}
try {
- // Get the checkpoint location
+ // Get the checkpoint location before closing
File checkpointLocation = null;
try {
- if (checkpointedManager.getStore() != null &&
+ if (checkpointedManager.getStore() != null &&
checkpointedManager.getStore().getDbLocation() != null) {
// The checkpoint location is typically the parent directory of the
DB location
checkpointLocation =
checkpointedManager.getStore().getDbLocation().getParentFile();
}
} catch (Exception e) {
LOG.warn("Failed to get checkpoint location for cleanup", e);
}
-
+
+ // Close the database connections first
+ checkpointedManager.stop();
Review Comment:
If this throws any rocksDB close related any exception by chance, then again
checkpoint dirs/files will not be cleaned. So my suggestion is to put in try
finally block like:
```suggestion
try {
checkpointedManager.stop();
LOG.debug("Closed checkpointed OM metadata manager database
connections");
} catch (Exception e) {
LOG.warn("Failed to stop checkpointed OM metadata manager", e);
} finally {
if (checkpointLocation != null && checkpointLocation.exists()) {
try {
FileUtils.deleteDirectory(checkpointLocation);
LOG.info("Cleaned up checkpoint directory: {}",
checkpointLocation);
} catch (IOException e) {
LOG.warn("Failed to cleanup checkpoint directory: {}",
checkpointLocation, e);
}
}
}
```
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/tasks/ReconTaskControllerImpl.java:
##########
Review Comment:
I would suggest to change this to info with some sampling, so that we don't
see lot of logs as well some info also available.
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/tasks/ReconTaskControllerImpl.java:
##########
Review Comment:
```suggestion
public synchronized ReInitializationResult queueReInitializationEvent(
ReconTaskReInitializationEvent.ReInitializationReason reason) {
LOG.info("Queueing task reinitialization event due to: {} (retry attempt
count: {})",
reason, eventProcessRetryCount.get());
controllerMetrics.incrTotalReprocessSubmittedToQueue();
ReInitializationResult reInitializationResult =
validateRetryCountAndDelay();
if (null != reInitializationResult) {
return reInitializationResult;
}
drainEventBufferAndCleanExistingCheckpoints();
ReconOMMetadataManager checkpointedOMMetadataManager = null;
boolean handedOff = false;
try {
LOG.info("Attempting checkpoint creation (retry attempt: {})",
eventProcessRetryCount.get() + 1);
try {
checkpointedOMMetadataManager =
createOMCheckpoint(currentOMMetadataManager);
LOG.info("Checkpoint creation succeeded");
} catch (IOException e) {
LOG.error("Checkpoint creation failed: {}", e.getMessage());
handleEventFailure();
return ReInitializationResult.RETRY_LATER;
}
ReconTaskReInitializationEvent reinitEvent =
new ReconTaskReInitializationEvent(reason,
checkpointedOMMetadataManager);
if (eventBuffer.offer(reinitEvent)) {
handedOff = true; // downstream consumer now owns
cleanup
resetEventFlags();
LOG.info("Successfully queued reinitialization event after {}
retries",
eventProcessRetryCount.get() + 1);
return ReInitializationResult.SUCCESS;
}
// Buffer full — drop the event and clean up the fresh checkpoint in
finally.
LOG.warn("Failed to queue reinitialization event (buffer full); "
+ "discarding fresh checkpoint at {}",
checkpointedOMMetadataManager != null
&& checkpointedOMMetadataManager.getStore() != null
? checkpointedOMMetadataManager.getStore().getDbLocation()
: "<unknown>");
handleEventFailure();
return ReInitializationResult.RETRY_LATER;
} finally {
if (!handedOff && checkpointedOMMetadataManager != null) {
cleanupCheckpoint(checkpointedOMMetadataManager);
}
}
}
```
--
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]
