Ensure ACKs are sent after backup #920
Closed
Conversation
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
added 2 commits
September 30, 2024 14:30
This commit addresses an issue where acknowledgments (ACKs) were sometimes sent to the master before binlog events were fully written and fsynced to disk during backup operations. Sending ACKs prematurely in semi-synchronous replication could lead to data loss if the replica fails after sending the ACK but before persisting the event. Key changes: - Introduced an `EventHandler` interface with a `HandleEvent` method for processing binlog events. This allows custom event handling logic to be injected into the replication stream. - Added an `eventHandler` field to `BinlogSyncer` and provided a `SetEventHandler` method to assign an event handler. This enables `BinlogSyncer` to delegate event processing to the assigned handler. - Implemented `BackupEventHandler` which writes binlog events to disk and ensures that each event is fsynced before returning. This ensures data durability before ACKs are sent. - Modified the `onStream` method in `BinlogSyncer` to separate event parsing (`parseEvent`) from event handling and ACK sending (`handleEventAndACK`). This adheres to the single-responsibility principle and makes the code cleaner. - Moved state updates (e.g., updating `b.nextPos`) and GTID set handling from `parseEvent` to `handleEventAndACK` to avoid side effects during parsing. - Ensured that ACKs are sent only after the event has been fully processed and fsynced by sending the ACK in `handleEventAndACK` after event handling.
…dling - Updated `backup_test.go` to ensure binlog files are written and fsynced before ACKs are sent by checking that backup files exist and have non-zero sizes after the backup process completes - Implemented `CountingEventHandler` in `backup_test.go` to confirm that the `EventHandler` is invoked during backups, ensuring events are processed as expected - Added `TestGTIDSetHandling` in `replication_test.go` to verify that GTIDSets are correctly handled after refactoring - Modified `testSync` method in `replication_test.go` to use a custom `EventHandler` and verify that events are processed correctly before ACKs are sent
dt8269
force-pushed
the
dterry/durablebinlogs
branch
from
b0273aa
to
1b08ee2
Compare
dt8269
changed the title
|
maybe we can limit the user to either use the new blocking handler or use the existing asynchronous streaming consumer. in your use case of persisting to disk, no need to bother draining the stream by GetEvent to achieve this, don't send the event to the channel if handler is set. and refine other logic and comments. |
…he event handler construction
dt8269
force-pushed
the
dterry/durablebinlogs
branch
7 times, most recently
from
1ff1b33
to
a23b791
Compare
dt8269
force-pushed
the
dterry/durablebinlogs
branch
from
a23b791
to
591a1c7
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
This PR addresses an issue where acknowledgments (ACKs) were sometimes sent to the master before binlog events were fully written and fsynced to disk during backup operations. Sending ACKs prematurely in semi-synchronous replication could lead to data loss if the replica fails after sending the ACK but before persisting the event.
ACK after backup
Event handling
EventHandlerinterface with aHandleEventmethod for processing binlog events. This allows custom event handling logic to be injected into the replication streameventHandlerfield andSetEventHandlermethod toBinlogSyncer.BackupEventHandlerwhich writes binlog events to disk and ensures that each event is fsynced before returning. This ensures data durability before ACKs are sent.Separating parsing from handling
onStreammethod inBinlogSyncerto separate event parsing (parseEvent) from event handling and ACK sending (handleEventAndACK). This adheres to the single-responsibility principle and makes the code cleanerb.nextPos) and GTIDSet handling fromparseEventtohandleEventAndACKto avoid side effects during parsing. Something calledparseEventshould only be parsing, not modifying state or sending ACKs.Testing
Three new tests:
TestGTIDSetHandling- This confirms the continued functionality of the existing GTIDSet logic after it was involved in a refactorTestACKSentAfterFsync- This confirms that ACKs are only send after the backup files are fsyncedTestBackupEventHandlerInvocation- This confirms that the (new) EventHandler works as expected