(Not upstreamed yet) Use async client in FullTableBackupClient#223
(Not upstreamed yet) Use async client in FullTableBackupClient#223hgromer wants to merge 3 commits intohubspot-2.6from
Conversation
|
This implementation doesn't attempt any unnecessary procedures |
| } | ||
|
|
||
| if (!admin.isSnapshotFinished(desc)) { | ||
| throw new IOException("Snapshot " + snapshotName + " not finished."); |
There was a problem hiding this comment.
Won't this still throw if the snapshot takes longer than waitMs?
There was a problem hiding this comment.
The initial problem I set out to solve was that the loop would re-try to take the snapshot on a later attempt and fail b/c the snapshot already exists. My initial fix is a bit clunky, and I thought this was cleaner
it's okay to throw if we timeout waiting for the snapshot to finish imo. we can up the config value if we want to. what do you think?
rmdmattingly
left a comment
There was a problem hiding this comment.
Main change looks good (use snapshotAsync + bounded wait to avoid Admin RPC timeouts), but a couple correctness/compat notes before I can approve:
-
Snapshot already exists / timeout race: old code returned early if snapshotName already exists (to handle cases where snapshot is still running but the Admin call timed out). New code will throw on timeout or on “snapshot already exists”. Suggest: on TimeoutException/ExecutionException, check if a snapshot with this name exists (or use isSnapshotFinished when possible) and treat that as success if finished.
-
Exception type: snapshotTable() declares
throws IOExceptionbut wraps all Exceptions in RuntimeException. Prefer rethrow as IOException (or propagate IOException directly) to keep callers consistent. -
SnapshotType.FLUSH: previously used
admin.snapshot(name, table)which may default to a different type in some versions. If FLUSH is intended, maybe add a short comment noting why.
Config: BACKUP_WAIT_MS_KEY + DEFAULT_BACKUP_WAIT_MS matches prior 10*10s behavior; just make sure release notes mention the key rename.
Use an async call to take the snapshot so we don't need to do any weird looping. this is simpler