anmolnar commented on PR #8845:
URL: https://github.com/apache/hbase/pull/8845#issuecomment-6667885098
> 🤖 **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 #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 #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 #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.
--
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]
