[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

[VladRodionov]

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

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:

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.

[anmolnar]

Changes looks good to me. Just a few comments.

[kgeisz]

Hey @anmolnar and @VladRodionov, I have addressed some additional comments again. These include:

• Removed log message verification in TestFullBackupMarkerFailure (per @anmolnar's suggestion).
• Updated docstring for HBackupFileSystem.writeBackupCompleteMarker().
  • The docstring no longer says the user can fallback to .backup.manifest if the .backup.complete marker file is missing.
  • It now says a backup can still be complete if the marker file is missing, but this can only be verified using the originating cluster's BackupSystemTable.
• During a backup merge, the .backup.complete marker file is not copied into the backup directory until the merge is actually complete.
• TestBackupRepair enhancement - Add a stale .backup.complete marker to a backup and verify a repair deletes the file.
• Updated TestIncrementalBackupMergeWithFailures.run() to match MapReduceBackupMergeJob.run() so it also waits until the merge is done to add a .backup.complete marker file.
  • Also verifies the marker file still exists after a failure before the merge operation.
  • Verifies the marker file does not exist after a failure during the actual merge.

[anmolnar]

🚢