Fix handling for suspend/resume case for PeriodFromFirst #88
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
mdedetrich
force-pushed
the
terminate-with-period-from-first
branch
from
b92c57b to
25d94e5
Compare
mdedetrich
commented
Jan 24, 2022
|
|
||
| override def empty: () => Future[Done] = () => Future.successful(Done) | ||
|
|
||
| /** Override this method to define how to backup a `ByteString` to a `DataSource` |
There was a problem hiding this comment.
This doc isn't doing anything useful since its already documented in BackupClientInterface and its also a test we are dealing with.
mdedetrich
force-pushed
the
terminate-with-period-from-first
branch
from
25d94e5 to
e89782d
Compare
mdedetrich
commented
Jan 24, 2022
| @@ -55,36 +56,61 @@ trait BackupClientInterface[T <: KafkaClientInterface] { | |||
| override val context: kafkaClientInterface.CursorContext | |||
| ) extends ByteStringElement | |||
|
|
|||
| case class PreviousState(state: State, previousKey: String) | |||
There was a problem hiding this comment.
These data structures have to be placed within BackupClientInterface because they reference State which is an abstract type member of the class itself
mdedetrich
force-pushed
the
terminate-with-period-from-first
branch
from
e89782d to
53f82d3
Compare
mdedetrich
force-pushed
the
terminate-with-period-from-first
branch
from
53f82d3 to
4514490
Compare
reta
approved these changes
Jan 25, 2022
|
@mdedetrich just curious what would happen if for some reason multiple instances of the backup happen to run concurrently (deployment error let say), could / do we detect that? |
|
Thanks for the approval, I also just added a commit that moves |
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.
About this change - What it does
This PR fixes the suspend/resume handling for the
FirstFromPeriodcase. As documented for theBackupconfiguration section inTimeConfiguration, when usingFirstFromPeriodif you kill a backup and then resume it, it will create a new object regardless of what the current configureddurationis as long as at least one chunk has been persisted into S3.The current behavior of Guardian does not do this, the PR fixes this.
In addition the PR also renames some tests so its much more clear what is actually being tested
Why this way
The intention of this PR is to keep the interface for
BackupClientInterfaceas simple as possible so that implementations ofBackupClientInterface(i.e. S3'sBackupClient) are trivial, simple and easy to understand even at a cost of internals ofBackupClientInterfacebeing more complicated.Here is a brief documentation of the public changes to the
BackupClientInterface*
backupToStorageTerminateSink: This newly added method is responsible for terminating an already existing backup which is needed forFirstFromPeriodto function as documented. The definedSinkis basically the same asbackupToStorageSinkhowever it doesn't have to deal with a Kafka context (since its not writing any Kafka messages) and it will always be resuming from a currently unfinished backup*
getCurrentUploadState: This method has been updated so that it returns the necessary information to implement this PR. It not only returns the state for the current key (if it exists) but it also returns the state for the last previous key before the current one (namedpreviousKey) along with the key itself.BackupClientInterfacehave been greatly improved.Additional changes
CurrentStatehas been renamed toStateto avoid confusion since we are dealing with both current previous states.TBD