[kgeisz]

https://issues.apache.org/jira/browse/HBASE-30526

AI Usage

Assisted with Claude Opus 4.6

Summary

This PR is a child task of HBASE-30517 (PR #8835). The goal of PR #8835 is to make it so the CustomBackupLocationPitrRestoreHandler.getBackupMetadata() method only gathers PITR metadata for backups in a COMPLETE state. However, as noted by one of the review comments in that PR, the current proposed implementation incorrectly relies on the BackupSystemTable.

Imagine a scenario where Cluster A has created backups stored in an external storage location. If Cluster B wants to restore one of these backups, then it would need to use the BackupSystemTable to retrieve each backup's state and filter out the non-COMPLETE backups. The problem is Cluster B's BackupSystemTable may not be the same as Cluster A's. With the changes in this PR, Cluster B can use the .backup.complete file in each relevant backup's directory to determine if the backup is in a COMPLETE state.

[anmolnar]

🤖 AI-assisted review — drafted by Claude on behalf of @anmolnar.

Thanks for picking this up! Using a marker file next to .backup.manifest makes sense for the cross-cluster case. I have a few concerns before this goes in, mostly about how marker failures interact with the existing failure path.

1. A failed marker write can wipe a backup that's already committed

In completeBackup(), the marker is written after the snapshots are deleted and after updateBackupInfo() has persisted COMPLETE. If fs.create() throws (a transient HDFS/S3 error, permissions, etc.), we land in failBackup():

• The phase is still STORE_MANIFEST, so cleanupTargetDir() deletes the table data directories.
• cleanupAndRestoreBackupSystem() then tries to delete or restore snapshots that are already gone, which will likely throw.

So a transient error can leave the system table saying COMPLETE (or ask for a repair) while the data is gone. Two options:

• (a) Make the marker best-effort: catch, LOG.warn, and don't fail the backup. A missing marker only makes the backup look incomplete to other clusters, which is the safe direction.
• (b) (my preference) Write the marker right after addManifest(), before the snapshot deletes and updateBackupInfo(), so a failure goes through the normal failure path. This needs 0.94 #2.

2. Failure cleanup and repair don't remove the marker

cleanupTargetDir() only deletes the <backupId>/<ns>/<table> directories, so anything at the top of <backupId>/ survives. Could it also delete <backupId>/.backup.complete? Repair goes through cleanupAndRestoreBackupSystem(), so it would pick this up too. That gives us the invariant: if the marker exists, the backup is good.

3. Backups taken before this change

Once #8835 filters on the marker, every backup taken before the upgrade drops out of PITR on other clusters. We should decide how to handle that, either here or explicitly in #8835 or the JIRA: a fallback, a tool that writes markers for existing COMPLETE backups, or at least a release note.

Smaller items

• The writeBackupCompleteMarker(...) signature line is over 100 characters, so spotless:check will probably flag it.
• Suggest adding a reader helper next to the writer, e.g. HBackupFileSystem.isBackupComplete(conf, rootDir, backupId) or getBackupCompleteMarkerPath(...), so HBASE-30517: Non-COMPLETE backups can be used in Point-In-Time Restore with a custom backup location #8835 and the tests share one definition of the path.
• Tests I'd like to see:
  • a failed backup (via BACKUP_TEST_MODE_STAGE / failStageIf) has no marker;
  • the merged backup still has its marker after a merge (TestBackupMerge); this is the one non-trivial path the PR changes;
  • repair removes a leftover marker, if 0.94 #2 is adopted.
• assertBackupCompleteFileExists would fit better in TestBackupBase than in TestBackupUtils, since several other test classes now depend on it.
• Nits:
  • in copyMetaData, p.getName().equals(BACKUP_COMPLETE_FILE_NAME) is more precise than indexOf(...) > 0;
  • "locaed" is still a typo two lines below the Javadoc you fixed;
  • consider writing the backup ID and completion timestamp into the marker to help debugging (optional).

One note for the record: during a merge's phase 2, the merged directory briefly has no marker between the rename to tmp and copyMetaData(). That seems like the right behavior, since a half-merged image shouldn't look complete. A short comment there would help.

[VladRodionov]

Yet another one review from a different AI God :).

Code Review: HBASE-30526 — Backup Completion Marker

PR: [apache/hbase#8845](#8845)
Related PR: [apache/hbase#8835](#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:

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:

<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:

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:

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:

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:

public static Path getBackupCompleteMarkerPath(
    String backupRootDir, String backupId)

Optionally, provide:

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.

[kgeisz]

Hey @anmolnar and @VladRodionov, I have addressed some of the review suggestions you shared. Can I get another round of reviews.

IMO, the biggest concern was an error occurring when creating .backup.complete and resulting in the backup failing. I chose the "best effort" approach that catches the exception in HBackupFileSystem.writeBackupCompleteMarker().

Some items are still unaddressed, and I will state my reasoning:

1. Marker creation is not atomic - .backup.complete is just an empty file, so it's creation is already atomic
2. Compatibility with existing backups is unresolved - Continuous Backup and PITR is a new feature. IMO we don't need to worry about existing backups not having the .backup.complete file since PITR does not apply to non-continuous backups that have been around already.
3. Write the backup ID and completion timestamp into .backup.complete file - IMO this is not necessary