devmadhuu commented on code in PR #10950:
URL: https://github.com/apache/ozone/pull/10950#discussion_r3718792006
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/tasks/ReconTaskControllerImpl.java:
##########
@@ -924,41 +938,50 @@ 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;
}
+ // Get the checkpoint location before closing.
+ File checkpointLocation = null;
try {
- // Get the checkpoint location
- File checkpointLocation = null;
- try {
- 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);
+ 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();
}
-
- // Clean up the checkpoint files if we have the location
+ } catch (Exception e) {
+ LOG.warn("Failed to get checkpoint location for cleanup", e);
+ }
+
+ // Close the database connections first, but always attempt to delete the
+ // checkpoint files afterwards - even if stop() throws - so the directory
+ // (a full copy of the OM DB) is never leaked.
+ try {
+ checkpointedManager.stop();
Review Comment:
Write a test when stop throws error and checkpoint still cleans up. that is
the purpose of new code.
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/tasks/ReconTaskControllerImpl.java:
##########
@@ -633,30 +636,43 @@ public synchronized
ReconTaskController.ReInitializationResult queueReInitializa
// Try checkpoint creation (single attempt per iteration)
ReconOMMetadataManager checkpointedOMMetadataManager = null;
-
+ // Whether the checkpoint has been handed off to the event buffer. If not,
+ // this method owns its cleanup (the finally block below).
+ boolean handedOff = false;
+
try {
LOG.info("Attempting checkpoint creation (retry attempt: {})",
eventProcessRetryCount.get() + 1);
- checkpointedOMMetadataManager =
createOMCheckpoint(currentOMMetadataManager);
- LOG.info("Checkpoint creation succeeded");
- } catch (IOException e) {
- LOG.error("Checkpoint creation failed: {}", e.getMessage());
+ try {
+ checkpointedOMMetadataManager =
createOMCheckpoint(currentOMMetadataManager);
+ LOG.info("Checkpoint creation succeeded");
+ } catch (IOException e) {
+ LOG.error("Checkpoint creation failed: {}", e.getMessage());
+ handleEventFailure();
+ return ReInitializationResult.RETRY_LATER;
+ }
+
+ // Create and queue the reinitialization event with checkpointed
metadata manager
+ ReconTaskReInitializationEvent reinitEvent =
+ new ReconTaskReInitializationEvent(reason,
checkpointedOMMetadataManager);
+ // If reinitialization event queued successfully, reset event buffer
overflow flag and task failure flag,
+ // so that we can resume queuing the delta events.
+ if (eventBuffer.offer(reinitEvent)) {
+ // The downstream consumer now owns the checkpoint and its cleanup.
+ handedOff = true;
+ resetEventFlags();
+ LOG.info("Successfully queued reinitialization event after {}
retries", eventProcessRetryCount.get() + 1);
+ return ReconTaskController.ReInitializationResult.SUCCESS;
+ }
+
+ // Buffer full - drop the event and clean up the fresh checkpoint (in
finally) to avoid leaking it.
+ LOG.warn("Failed to queue reinitialization event (buffer full);
discarding fresh checkpoint");
Review Comment:
Its better, we we log the dblocation of checkpoint also here for failure
case.
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/tasks/ReconTaskControllerImpl.java:
##########
@@ -633,30 +636,43 @@ public synchronized
ReconTaskController.ReInitializationResult queueReInitializa
// Try checkpoint creation (single attempt per iteration)
ReconOMMetadataManager checkpointedOMMetadataManager = null;
-
+ // Whether the checkpoint has been handed off to the event buffer. If not,
+ // this method owns its cleanup (the finally block below).
+ boolean handedOff = false;
+
try {
LOG.info("Attempting checkpoint creation (retry attempt: {})",
eventProcessRetryCount.get() + 1);
- checkpointedOMMetadataManager =
createOMCheckpoint(currentOMMetadataManager);
- LOG.info("Checkpoint creation succeeded");
- } catch (IOException e) {
- LOG.error("Checkpoint creation failed: {}", e.getMessage());
+ try {
+ checkpointedOMMetadataManager =
createOMCheckpoint(currentOMMetadataManager);
+ LOG.info("Checkpoint creation succeeded");
+ } catch (IOException e) {
+ LOG.error("Checkpoint creation failed: {}", e.getMessage());
+ handleEventFailure();
+ return ReInitializationResult.RETRY_LATER;
+ }
+
+ // Create and queue the reinitialization event with checkpointed
metadata manager
+ ReconTaskReInitializationEvent reinitEvent =
+ new ReconTaskReInitializationEvent(reason,
checkpointedOMMetadataManager);
+ // If reinitialization event queued successfully, reset event buffer
overflow flag and task failure flag,
+ // so that we can resume queuing the delta events.
+ if (eventBuffer.offer(reinitEvent)) {
+ // The downstream consumer now owns the checkpoint and its cleanup.
+ handedOff = true;
+ resetEventFlags();
+ LOG.info("Successfully queued reinitialization event after {}
retries", eventProcessRetryCount.get() + 1);
+ return ReconTaskController.ReInitializationResult.SUCCESS;
+ }
+
+ // Buffer full - drop the event and clean up the fresh checkpoint (in
finally) to avoid leaking it.
+ LOG.warn("Failed to queue reinitialization event (buffer full);
discarding fresh checkpoint");
Review Comment:
When code reaches here, means buffer full, please add a test case for this
condition also.
--
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]
