Fix Create Backup Consumption Logic #542
Merged
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
Fleshgrinder
force-pushed
the
fleshgrinder/backup-watermarks
branch
from
65d7655
to
e04b85f
Compare
Deploying with
|
| Latest commit: |
08e89d7
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e31802c9.karapace.pages.dev |
| Branch Preview URL: | https://fleshgrinder-backup-watermar.karapace.pages.dev |
Fleshgrinder
force-pushed
the
fleshgrinder/backup-watermarks
branch
3 times, most recently
from
10e01fe
to
6425450
Compare
Fleshgrinder
force-pushed
the
fleshgrinder/backup-watermarks
branch
from
6425450
to
733d853
Compare
Fleshgrinder
force-pushed
the
fleshgrinder/backup-watermarks
branch
5 times, most recently
from
f886f87
to
c6b1e94
Compare
Comment on lines
+402
to
+404
| last_offset = record.offset | ||
| if last_offset >= end_offset: | ||
| break |
There was a problem hiding this comment.
I'd compare the record offset to end offset in the loop and break immediately when reached. Now this could backup more than was decided as the end offset.
There was a problem hiding this comment.
Which is intentional, backing up more than the minimum is totally fine. Using a while loop would mean that we perform the same check twice in a row, however, using the do-while loop means that we perform only as many checks as necessary, while allowing to back up as much as possible, but at least what was in the topic when we started.
Fleshgrinder
force-pushed
the
fleshgrinder/backup-watermarks
branch
from
c6b1e94
to
9b68d37
Compare
Fleshgrinder
requested review from
antonagestam and
jjaakola-aiven
and removed request for
antonagestam
Fleshgrinder
force-pushed
the
fleshgrinder/backup-watermarks
branch
from
9b68d37
to
2d89696
Compare
The consumption logic is currently counting how many records it received in a single batch returned from poll, and when it is empty it concludes that the backup is successfully finished. However, there are meany reasons why a batch returned by poll is empty, especially with timeouts applied to it. A consequence of this is that a backup created at $t_1$ may contain more records than a backup created at $t_2$ (without any external changes to the topic content, e.g. compaction). To fix this we have to use offset watermarks. With them we can determine if we are done, or not. The patch now exposes the poll timeout, so that users can increase it in case they encounter issues, and it uses a longer default poll timeout to ensure that users are not going to see errors right away (increased from 1 second to 1 minute).
Fleshgrinder
force-pushed
the
fleshgrinder/backup-watermarks
branch
from
2d89696
to
08e89d7
Compare
jjaakola-aiven
approved these changes
Feb 22, 2023
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.
The consumption logic is currently counting how many records it received in a single batch returned from poll, and when it is empty it concludes that the backup is successfully finished. However, there are meany reasons why a batch returned by poll is empty, especially with timeouts applied to it. A consequence of this is that a backup created at$t_1$ may contain more records than a backup created at $t_2$ (without any external changes to the topic content, e.g. compaction).
To fix this we have to use offset watermarks. With them we can determine if we are done, or not. The patch now exposes the poll timeout, so that users can increase it in case they encounter issues, and it uses a longer default poll timeout to ensure that users are not going to see errors right away (increased from 1 second to 1 minute).
Requires (and includes) #540 to be merged first.