Reunite checkpoint and backup core logic #1932
Conversation
|
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@ajkr updated the pull request - view changes - changes since last import |
ajkr
force-pushed
the
backup-using-checkpoint
branch
from
3bb64a7
to
12e88c0
Compare
|
@ajkr updated the pull request - view changes - changes since last import |
|
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@ajkr do you plan to rebase? |
|
Yes, maybe will context switch back to this next week, I have some other things to work on now. |
|
@ajkr makes sense. The next week is the TechDebt week. |
ajkr
force-pushed
the
backup-using-checkpoint
branch
from
12e88c0
to
32df5e4
Compare
|
@ajkr updated the pull request - view changes - changes since last import |
|
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@ajkr updated the pull request - view changes - changes since last import |
|
ping |
|
Can you resolve the conflict? |
|
yes as long as you intend to review (otherwise it's kinda pointless to repeatedly resolve merge conflicts). |
ajkr
force-pushed
the
backup-using-checkpoint
branch
from
677649e
to
e1fc1b4
Compare
|
@ajkr updated the pull request - view changes - changes since last import |
| FileType) { | ||
| // custom checkpoint will switch to calling copy_file_cb after it sees | ||
| // NotSupported returned from link_file_cb. | ||
| return Status::NotSupported(); |
There was a problem hiding this comment.
Sounds like something like internal error.
There was a problem hiding this comment.
I agree, but this reuses the logic that checkpoint already has for using file copying on Envs that don't support linking. Do you have an alternative idea?
|
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
ajkr
force-pushed
the
backup-using-checkpoint
branch
from
e1fc1b4
to
270349f
Compare
|
@ajkr updated the pull request - view changes - changes since last import |
|
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@ajkr updated the pull request - view changes - changes since last import |
|
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
1 similar comment
|
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
These code paths forked when checkpoint was introduced by copy/pasting the core backup logic. Over time they diverged and bug fixes were sometimes applied to one but not the other (like fix to include all relevant WALs for 2PC), or it required extra effort to fix both (like fix to forge CURRENT file). This diff reunites the code paths by extracting the core logic into a function, CreateCustomCheckpoint(), that is customizable via callbacks to implement both checkpoint and backup.
Related changes: