Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use bookmarks created after the latest snapshot #625

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

0xFelix
Copy link
Contributor

@0xFelix 0xFelix commented Mar 14, 2021

This commit changes syncoid's behavior so it is always looking for
a matching snapshot and a matching bookmark. If the bookmark was created
after the snapshot it is used instead. This allows replication when the
latest snapshot replicated was deleted on the source, a common snapshot
was found but rollback on the target is not allowed. The matching bookmark
is used instead for replication.

This fixes #602

@yhaenggi
Copy link

gentle bump

@msladek
Copy link

msladek commented Jan 24, 2022

would love to see this PR reviewed & merged @jimsalterjrs if time allows for it

@0xFelix 0xFelix force-pushed the bookmarks-after-latest-snapshot branch from ab34d78 to 0232319 Compare April 23, 2023 10:08
@0xFelix
Copy link
Contributor Author

0xFelix commented Apr 23, 2023

@jimsalterjrs @phreaker0 Ping

@0xFelix 0xFelix force-pushed the bookmarks-after-latest-snapshot branch 3 times, most recently from d93bc7c to ca7fb68 Compare May 17, 2023 10:23
@0xFelix 0xFelix force-pushed the bookmarks-after-latest-snapshot branch from ca7fb68 to 3f6149a Compare July 27, 2023 12:25
@msladek
Copy link

msladek commented Apr 2, 2024

@0xFelix sadly merge conflicts have creeped in again due to lots getting merged except this... I still rely on this PR in order to be able to use syncoid at all and would be willing to resolve the conflicts if I had write access to your repo. just let me know.

@0xFelix
Copy link
Contributor Author

0xFelix commented Apr 2, 2024

@msladek Actually, I planned to pick up work on this and my other PR again soon, but help is always appreciated. Will contact you tomorrow.

@0xFelix 0xFelix force-pushed the bookmarks-after-latest-snapshot branch 4 times, most recently from a47c12f to f39f1a9 Compare April 20, 2024 12:11
Before this sortsnapshots always tried to sort source snapshots, also
when it was requested to sort target snapshots. This led to errors
when the list of target and source snapshots was not identical.
Fix it by allowing to specify an index when sorting snapshots.
@0xFelix 0xFelix force-pushed the bookmarks-after-latest-snapshot branch from f39f1a9 to 89b98ce Compare April 20, 2024 12:59
@0xFelix
Copy link
Contributor Author

0xFelix commented Apr 20, 2024

Rebased and working again.

Ping @phreaker0

@0xFelix
Copy link
Contributor Author

0xFelix commented Apr 20, 2024

Also ping @jimsalterjrs

@0xFelix 0xFelix force-pushed the bookmarks-after-latest-snapshot branch from 89b98ce to 8e50a73 Compare April 21, 2024 09:47
This commit changes syncoid's behavior so it is always looking for
a matching snapshot and a matching bookmark. If the bookmark was created
after the snapshot it is used instead. This allows replication when the
latest snapshot replicated was deleted on the source, a common snapshot
was found but rollback on the target is not allowed. The matching bookmark
is used instead for replication.

This fixes jimsalterjrs#602
@0xFelix 0xFelix force-pushed the bookmarks-after-latest-snapshot branch from 8e50a73 to 0266618 Compare April 21, 2024 09:51
Copy link
Collaborator

@phreaker0 phreaker0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this pr needs a rebase again, i just looked through and it looks good but I didn't test it.

I only have one concern, with this PR the bookmark list call is made for every dataset and may slow down the process for people which don't use the bookmark feature. Can you add a "no-bookmark" option to prevent the use of bookmarks?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants