VladRodionov commented on PR #8845: URL: https://github.com/apache/hbase/pull/8845#issuecomment-6755027517
**PR:** [[apache/hbase#8845](https://github.com/apache/hbase/pull/8845)](https://github.com/apache/hbase/pull/8845) **Reviewed revision:** `2f889f0` **Related PR:** [[apache/hbase#8835](https://github.com/apache/hbase/pull/8835)](https://github.com/apache/hbase/pull/8835) **Original review:** ChatGPT 5.6 Sol **Follow-up review:** ChatGPT (GPT-6) **Recommendation:** Request changes ## Summary Thank you for addressing the previous review comments. The updated implementation represents substantial progress. Most of the original concerns have been resolved, particularly the possibility that marker creation failures could invalidate an already committed backup. The additional negative tests and improvements to backup cleanup are also welcome. However, I still see two correctness concerns that should be addressed before approval. ## 1. Status of Previous Review Comments | Issue | Status | |---|---| | Marker write failure can invalidate a committed backup | Resolved | | Failure cleanup does not remove stale markers | Addressed | | Marker creation is not atomic | Partially addressed | | Compatibility with existing backups | Accepted with qualification | | Missing negative and failure tests | Substantially improved | | Incorrect marker filename matching during merge | Resolved | | Missing shared marker existence helper | Resolved | | Code formatting and documentation | Mostly resolved | ### Marker creation failure — Resolved Making marker creation best-effort is a reasonable solution. A marker write failure should not destroy an otherwise valid backup already committed to `BackupSystemTable`. The new `TestFullBackupMarkerFailure` provides meaningful coverage for this scenario. ### Failure cleanup — Addressed Adding explicit marker deletion to `cleanupTargetDir()` is the correct improvement. The additional assertions in `TestFullBackupWithFailures` and `TestBackupRepair` are useful. One remaining test improvement would be to explicitly create a stale marker before invoking repair and verify that repair removes it. This is not a separate blocker. ## 2. [Major] Manifest fallback contradicts the purpose of the completion marker **File:** `HBackupFileSystem.java` **Method:** `writeBackupCompleteMarker()` The updated Javadoc suggests falling back to checking for a valid `BackupManifest` when the completion marker is absent. I disagree with this recommendation. The original problem in HBASE-30517 is precisely that the existence of a backup manifest does not guarantee successful backup completion. In `TableBackupClient.completeBackup()`, the manifest is written by `addManifest()` before the backup state is persisted as `COMPLETE`. Therefore, the following sequence is possible: 1. Backup data copying finishes. 2. `addManifest()` successfully writes the manifest. 3. A subsequent finalization operation fails. 4. The backup remains incomplete or requires recovery. 5. The filesystem nevertheless contains a valid manifest. If custom-location PITR falls back to manifest existence, it may select an incomplete backup. This effectively reintroduces the original bug. ### Recommendation The completion state should distinguish between: - **Marker exists:** The backup has been marked complete. - **Marker absent:** Successful completion cannot be verified using the marker. - **Filesystem I/O failure:** Completion status cannot be determined. For newly created backups, marker absence must not automatically authorize their use in PITR. If compatibility with older backups is required, it should be handled explicitly rather than treating every readable manifest as evidence of successful completion. I agree that backward compatibility may be deferred if continuous backup and PITR have not been released to users. However, the semantics of the completion marker must remain consistent. **A valid manifest is not equivalent to a completed backup.** ## 3. [Major] Completion marker can be published before backup merge finishes **File:** `MapReduceBackupMergeJob.java` **Methods:** `run()`, `copyMetaData()` The revised `copyMetaData()` correctly recognizes `.backup.complete` using an exact filename comparison. However, the marker publication timing remains concerning. The merge procedure follows approximately this sequence: ```text 1. Rename existing backup directory to a temporary location 2. Move merged data into the destination 3. Update the backup manifest 4. Copy metadata back, including .backup.complete 5. Delete the temporary directory 6. Delete obsolete backup images 7. Finish the merge operation 8. Release the exclusive backup lock ``` The completion marker is restored during step 4, before the merge has finished. Furthermore, `copyMetaData()` uses `fs.listFiles()` to enumerate files and copies them in iteration order. There is no guarantee that `.backup.complete` is copied after all other metadata files. Consequently, the marker may become visible before the destination metadata has been fully restored. Consider a concurrent PITR operation from another cluster: 1. The merge copies `.backup.complete` into the destination. 2. The external cluster discovers the marker. 3. The external cluster considers the backup complete. 4. The merge has not yet finished restoring metadata or finalizing its state. A failure after marker publication but before merge completion creates a similar problem. The merge repair mechanism may eventually restore consistency, but it cannot prevent a concurrent external reader from observing the intermediate state. ### Recommendation Treat the completion marker as a **publication boundary** for the merged backup. I suggest: 1. Exclude `.backup.complete` from the regular metadata-copy loop. 2. Copy and finalize all other metadata first. 3. Publish the marker only after all operations necessary to make the merged backup independently restorable have succeeded. 4. Ensure that merge failure and repair paths cannot expose an incomplete merged backup as complete. The marker does not necessarily need to be the final operation in the entire merge procedure, but it must be published after the merged image becomes independently consistent and restorable. I would also recommend a failure-injection test that forces an exception during metadata copying or merge finalization, verifying that an incomplete merged image is never advertised as complete. This is especially important because custom-location PITR cannot depend on the originating cluster's backup lock or system table. ## 4. [Minor] Marker creation atomicity and filesystem semantics **File:** `HBackupFileSystem.java` The implementation uses: ```java fs.create(markerPath, true).close(); ``` The author noted that creating an empty file is already atomic. However, this assumption is not universally valid across Hadoop-compatible filesystems. HDFS and object stores have different file creation, close, and visibility semantics. The fact that the marker contains no data simplifies the implementation, but it does not establish universal atomic-publication guarantees. I do not necessarily recommend introducing temporary-file-and-rename logic, since rename operations themselves are not atomic on many object stores. Instead, the required consistency semantics should be explicitly documented. For ordinary backup completion, the current best-effort approach seems reasonable because the marker is written after the backup state has been committed. For backup merge, however, the publication ordering discussed above remains important. **I consider this nonblocking if the merge publication issue is addressed and the supported filesystem assumptions are documented.** ## 5. Test Coverage The updated tests substantially improve coverage: - `TestFullBackupMarkerFailure` verifies that a marker creation error does not invalidate a completed backup. - `TestFullBackupWithFailures` checks marker absence after failures. - `TestBackupRepair` includes marker-related assertions. - `TestBackupMerge` checks marker preservation for the surviving merged backup. - `TestHBackupFileSystem` adds filesystem-level coverage. I recommend the following additional tests: 1. **Stale marker repair:** Explicitly create a stale completion marker, invoke backup repair, and verify its removal. 2. **Merge failure:** Inject an exception during metadata copying or finalization and verify that an incomplete merged backup is not advertised as complete. 3. **PITR integration:** Verify that a manifest without a completion marker is not sufficient to classify a newly created backup as complete. The first is a useful enhancement. The second and third are important for ensuring the correctness of the completion-marker protocol. --- ## Final Assessment The author has addressed most of the substantive issues from the original review. I am satisfied with: - Best-effort marker creation after normal backup completion. - Explicit marker cleanup during backup failure handling. - Improved marker filename matching. - The shared filesystem helper. - The additional negative tests. Two concerns remain: **1. Manifest fallback:** A valid manifest must not be treated as unconditional proof of successful backup completion. Doing so would undermine the purpose of HBASE-30517. **2. Marker publication during merge:** The completion marker must not become visible before the merged backup is independently consistent and restorable. The second concern is particularly important because external PITR clients may not have access to the originating cluster's synchronization or backup metadata. I recommend addressing these two issues before merging. Once the marker semantics and merge publication ordering are corrected, I see no reason to block this PR over the remaining minor implementation details. **Overall recommendation: Request changes.** --- *Original AI-assisted review by ChatGPT 5.6 Sol. Follow-up review by ChatGPT (GPT-6), based on static inspection of PR #8845 through commit `2f889f0` and relevant Apache HBase source 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]
