VladRodionov commented on PR #8835: URL: https://github.com/apache/hbase/pull/8835#issuecomment-6618968543
**PR Review by ChatGPT 5.6 Sol** ## Review Summary The PR addresses a valid correctness issue: incomplete backups must not be selected for Point-in-Time Restore (PITR). However, I have two concerns with the proposed implementation, including a potential regression in custom backup location support. ### 1. [Blocking] Custom backup locations now depend on the current cluster's BackupSystemTable `CustomBackupLocationPitrRestoreHandler.java` Previously, the custom-location handler discovered backup images directly from filesystem manifests. With this change, each image must also have a corresponding `BackupInfo` entry in the current cluster's `BackupSystemTable`, with state `COMPLETE`. This introduces a dependency that may break an important use case: 1. Cluster A creates a complete backup in an external filesystem location. 2. The backup is moved or made accessible to Cluster B. 3. Cluster B attempts PITR using the custom backup location. 4. Cluster B's `BackupSystemTable` does not contain the backup ID. 5. The handler silently excludes the backup. The backup manifest is designed to preserve the metadata needed for restoration, including support for relocated backups. Requiring the current cluster's system table to contain the original backup record appears inconsistent with this design. **Recommendation:** Preserve filesystem-based backup discovery without introducing a mandatory dependency on the current cluster's `BackupSystemTable`. Ideally, backup completeness should be verifiable from filesystem metadata, perhaps through an explicit completion marker written only after successful backup finalization. If this requires a larger change, we should at least clarify the intended compatibility guarantees and add a regression test covering backups created on another cluster or moved to a different location. ### 2. [Major] IOException must not be silently interpreted as an incomplete backup `CustomBackupLocationPitrRestoreHandler.java` — `isComplete()` The current implementation catches `IOException` from `readBackupInfo()` and returns `false`. This conflates three different conditions: - The backup exists but is not complete. - The backup metadata does not exist. - The system table cannot be read due to an I/O failure. A temporary I/O failure could silently exclude otherwise valid backup images, potentially producing misleading PITR errors. **Recommendation:** Propagate `IOException` rather than silently treating it as an incomplete backup. The default PITR handler already propagates metadata access errors, and the custom handler should follow the same approach. Since the stream predicate cannot directly throw a checked exception, replacing the stream with a conventional loop would simplify the implementation. ### 3. [Major] Missing compatibility regression tests `TestCustomBackupLocationPitrRestoreHandler.java` The new tests cover filtering by backup state, missing metadata entries, and I/O failures. However, they primarily validate the newly introduced behavior rather than preserving existing functionality. In particular, the test asserting that an image with no corresponding `BackupInfo` should be filtered out effectively codifies the compatibility concern described above. **Recommendation:** Add a test verifying that a complete backup discovered through a custom filesystem location remains usable when the current cluster does not have its original system-table metadata, provided the required PITR metadata and WALs are available. ### 4. [Minor] Repeated system-table lookups The handler performs a separate `readBackupInfo()` lookup for every discovered backup image. For a location containing many images, this results in N individual metadata lookups. If the system-table dependency is retained, consider retrieving the relevant backup history in a batch and filtering against an in-memory map. This is a minor optimization and not a merge blocker. --- ### Conclusion I agree with the objective of preventing incomplete backups from being selected for PITR. However, the implementation introduces a potentially breaking dependency between custom-location restoration and the current cluster's system-table metadata. **I recommend addressing the custom-location compatibility issue and IOException handling before merging.** The central architectural question is whether a backup stored in an external filesystem location should remain independently restorable without requiring its original `BackupSystemTable` records. The existing manifest-based design suggests that it should. -- 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]
