VladRodionov commented on PR #8845: URL: https://github.com/apache/hbase/pull/8845#issuecomment-6670119268
Yet another one review from a different AI God :). # Code Review: HBASE-30526 — Backup Completion Marker **PR:** [[apache/hbase#8845](https://github.com/apache/hbase/pull/8845)](https://github.com/apache/hbase/pull/8845) **Related PR:** [[apache/hbase#8835](https://github.com/apache/hbase/pull/8835)](https://github.com/apache/hbase/pull/8835) — HBASE-30517 **Review performed by:** ChatGPT 5.6 Sol (AI-assisted review) **Recommendation:** **Request changes** ## Summary This PR addresses the primary architectural concern raised during the review of HBASE-30517: determining whether a backup is complete without relying on the originating cluster's `BackupSystemTable`. Introducing a `.backup.complete` marker in the backup directory is a reasonable solution. It allows backup discovery and PITR validation to operate independently of the cluster where the backup was created. The implementation is small, and the additional tests cover marker creation for several backup types. However, the proposed changes introduce concerns around backup finalization, failure recovery, and the consistency of the completion marker with the actual backup state. ## 1. [Blocking] Marker creation failure can invalidate an already completed backup **File:** `TableBackupClient.java` **Method:** `completeBackup()` The marker is written after: ```java BackupSystemTable.deleteSnapshot(conn); backupManager.updateBackupInfo(backupInfo); HBackupFileSystem.writeBackupCompleteMarker( conf, backupInfo.getBackupRootDir(), backupInfo.getBackupId()); ``` At this point, the backup state has already been persisted as `COMPLETE`, and the backup system table snapshot has been deleted. If `writeBackupCompleteMarker()` throws an `IOException`, the backup completion procedure may enter its failure-handling path even though the backup has already been successfully committed. This creates several risks: - The system table may already report the backup as `COMPLETE`. - Failure cleanup may remove valid backup data. - Recovery may attempt to restore a system table snapshot that has already been deleted. - The backup metadata and filesystem contents may become inconsistent. **Recommendation:** The completion marker should be integrated into the backup commit protocol, with explicitly defined failure semantics. At minimum, marker creation must not trigger destructive rollback after backup metadata has been committed and its recovery snapshot removed. One possibility is to create the marker before the irreversible finalization steps, provided that all subsequent failure paths reliably remove it. Another is to treat marker creation failures as a separate recoverable finalization error. The correct solution depends on the desired durability guarantee: if marker existence is authoritative evidence of a completed backup, its lifecycle must be coordinated with backup commit and rollback operations. ## 2. [Blocking] Cleanup must remove stale completion markers **Files:** `TableBackupClient.java`, `HBackupFileSystem.java` The new marker is created directly under the backup ID directory: ```text <backupRoot>/<backupId>/.backup.complete ``` However, the existing backup cleanup logic was designed before this file existed. In particular, `cleanupTargetDir()` deletes backup table directories but does not explicitly delete the root-level completion marker. If a backup fails after marker creation, or a recovery operation rolls back a previously marked backup, the marker may survive while the actual backup data is removed or incomplete. This would violate the fundamental invariant required by HBASE-30517: > A completion marker must never identify an incomplete or invalid backup as complete. **Recommendation:** Ensure the marker is deleted during all applicable backup failure, rollback, and repair paths. Consider introducing centralized helpers for marker creation, deletion, and existence checks rather than distributing marker lifecycle management across several components. A failed or rolled-back backup must not retain a completion marker. ## 3. [Major] Marker creation is not atomic **File:** `HBackupFileSystem.java` **Method:** `writeBackupCompleteMarker()` The implementation uses: ```java fs.create(markerPath, true).close(); ``` There are two concerns: 1. The marker is created directly at its final location. 2. The `overwrite=true` argument allows an existing marker to be silently replaced. A failure during file creation or closing may leave a marker whose existence does not reliably indicate that finalization succeeded. Because the file is empty and the intended consumer will presumably check only for its existence, there is no way to distinguish a successfully committed marker from a partially created one. **Recommendation:** Consider a temporary-file-and-rename protocol: 1. Create a temporary marker in the same directory. 2. Close the output stream successfully. 3. Rename the temporary file to `.backup.complete`. 4. Clean up the temporary marker on failure. This provides a stronger publication boundary on filesystems supporting atomic rename. For object stores, the implementation must account for the specific filesystem's rename semantics; rename should not be assumed atomic universally. Also consider whether overwriting an existing completion marker should be permitted. Backup IDs should normally be unique, so an existing marker might indicate an unexpected state rather than something to overwrite. ## 4. [Major] Compatibility with existing backups is unresolved **Related PR:** HBASE-30517 (#8835) Backups created before this change will not contain `.backup.complete`. If HBASE-30517 subsequently treats marker existence as a mandatory condition for backup selection, previously created backups may disappear from custom-location PITR discovery, even though they remain valid and restorable. This could become a backward-compatibility regression during rolling upgrades, cluster migrations, or disaster recovery. **Recommendation:** Define an explicit compatibility policy before relying on the marker for backup discovery. Possible approaches include: - A migration utility that generates markers for existing backups after verifying their state. - A compatibility fallback for legacy manifests, with clearly defined limitations. - A versioned marker requirement combined with documented upgrade procedures. Blindly treating every legacy backup without a marker as incomplete is not acceptable without acknowledging the compatibility impact. ## 5. [Major] Missing negative and recovery tests **Files:** Backup test classes The PR adds assertions verifying that the completion marker exists after successful full, incremental, and continuous backups. These are useful positive tests, but the most important correctness property is the opposite: **An incomplete or failed backup must never have a valid completion marker.** I recommend adding tests for: 1. **Backup failure:** Inject failures during backup execution and verify that no completion marker remains. 2. **Marker creation failure:** Simulate an `IOException` during marker creation and verify that backup metadata and filesystem contents remain consistent. 3. **Backup repair:** Verify that repair and rollback operations remove stale markers. 4. **Backup merge:** Verify that a successful merge preserves the completion marker and that failures during merge do not incorrectly expose an incomplete result as complete. 5. **Cross-cluster discovery:** Verify that marker-based backup validation works without access to the originating cluster's `BackupSystemTable`. The existing `BACKUP_TEST_MODE_STAGE` mechanism can be reused for some of these tests. ## 6. [Minor] Completion marker handling during backup merge **File:** `MapReduceBackupMergeJob.java` **Method:** `copyMetaData()` Adding `.backup.complete` to the metadata files preserved during merge is necessary. However, the current implementation uses: ```java fileName.indexOf( HBackupFileSystem.BACKUP_COMPLETE_FILE_NAME) > 0 ``` This matches any path containing the marker name, rather than strictly identifying the expected marker file. Prefer checking the filename directly: ```java HBackupFileSystem.BACKUP_COMPLETE_FILE_NAME.equals(p.getName()) ``` It would also be useful to document the marker's state during merge. The marker is temporarily moved with the original backup directory and subsequently copied back. This means the merged backup directory does not initially advertise completion, which is desirable. However, the marker is restored before all merge operations are finalized. We should verify that the failure and repair paths preserve the intended consistency guarantees. ## 7. [Minor] Centralize marker path construction **File:** `HBackupFileSystem.java` The marker path is currently constructed separately by production code and test utilities. Since HBASE-30517 will also need to locate the marker, I suggest introducing a helper such as: ```java public static Path getBackupCompleteMarkerPath( String backupRootDir, String backupId) ``` Optionally, provide: ```java public static boolean isBackupComplete( Configuration conf, String backupRootDir, String backupId) throws IOException ``` This avoids duplicating path construction and establishes a single definition of the completion marker. The existence-check helper should propagate filesystem errors rather than silently interpreting them as a missing marker. ## 8. [Minor] Code quality and test organization A few smaller improvements: - The `writeBackupCompleteMarker()` method declaration should be wrapped to comply with HBase's line-length conventions. - Consider moving `assertBackupCompleteFileExists()` from `TestBackupUtils` to `TestBackupBase`, since it is now shared by several backup test classes. - Include the marker path in assertion failure messages to simplify debugging. - The Javadoc in `MapReduceBackupMergeJob.copyMetaData()` still contains the typo `locaed`. - An optional completion timestamp or backup ID in the marker could improve operational diagnostics, although an empty marker is sufficient if its lifecycle is reliable. --- ## Final Assessment I support the architectural direction of this change. A filesystem-based completion marker is a better solution than consulting the current cluster's `BackupSystemTable`, particularly for cross-cluster backup restoration. However, **the completion marker becomes part of the backup's persistent correctness contract**, not merely an auxiliary metadata file. Its creation, publication, deletion, and recovery semantics must therefore be designed carefully. Before merging, I recommend addressing three primary issues: 1. **Fix the backup finalization failure window** so that marker creation failures cannot trigger destructive cleanup after backup commit. 2. **Ensure rollback and repair remove stale markers**, preserving the invariant that an incomplete backup cannot be advertised as complete. 3. **Add negative and failure-injection tests**, particularly for marker creation failure and backup merge recovery. The backward compatibility of pre-existing backups should also be explicitly resolved before HBASE-30517 starts requiring these markers. **Overall recommendation: Request changes.** The implementation is heading in the right direction, but the marker lifecycle needs stronger transactional guarantees before it can safely serve as authoritative evidence of backup completion. --- *Review performed with ChatGPT 5.6 Sol. Static code review of PR #8845 and relevant Apache HBase code; tests were not executed.* -- 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]
