-
Notifications
You must be signed in to change notification settings - Fork 998
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
Ensure ACKs are sent after backup #921
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
c7a3c17
This commit addresses an issue where acknowledgments (ACKs) were some…
d162219
Refactor event handling by replacing SyncMode and EventHandleMode wit…
f04825e
Merge branch 'master' into dterry/durablebinlogs
dt8269 ca576c9
Add some comments and remember to remove SetEventHandler and the even…
a8bcf4c
Remove the timeout for synchronous backup, revert the timeout move to…
06c9268
Make sure to assign the timeout on the syncer so the backup doesn't fail
bc1dd07
Make sure to add NewBackupHandler in order to expose the otherwise pr…
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't need to change
StartBackupWithHandler
now? Because your requirement does not useStartBackupWithHandler
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the logic of writing on disk is contained in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the old code, we still can use
handler
to createio.WriteCloser
and write events to disk.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, but I don't want to duplicate the code in
HandleEvent
. Do you want me to remove theBackupEventHandler
struct and haveHandleEvent
be static?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I don't know where is the duplication. Can you elaborate?
No I don't mean that. I mean reverting
StartBackupWithHandler
to old code. YourStartSynchronousBackup
is fine. AndBackupEventHandler
seems no need to be public (exported)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
StartBackupWithHandler
implemented the code inHandleEvent
which is also used by theSynchronousEventHandler
inreplication/binlogsyncer.go
. As a helpful shorthand to prevent the need for clients to reimplementHandleEvent
in their code, we can leave it extracted as its own method.For example, if we reverted
StartBackupWithHandler
we'd either have that code (if e.Header.EventType == ROTATE_EVENT
...) duplicated inHandleEvent
or, if we removed that as well (as you suggested) we'd have it duplicated in the test code. Does that make sense?If you have a different idea of how this should all change, can you upload an example diff?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see the duplication is
if e.Header.EventType == ROTATE_EVENT
, thanks. I think it's reasonable.And here's my suggestions, please check them.
git.diff.txt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we make
newBackupEventHandler
private, clients won't be able to do what backup_test.go (an example client) is doingThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if they are the same behaviour as BackupEventHandler, they can directly use SynchronousEventHandler. if they have more customised behaviour, current BackupEventHandler is not extendable to other behaviour I think (no place to inject new logic?). so they still need to write many lines of code.maybe you think they can wrap a structure on BackupEventHandler, dispatch event by event type to BackupEventHandler or the parent structure? I can't think of a real use case for now.oh you are right 👍