-
Notifications
You must be signed in to change notification settings - Fork 8
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
Implement CLI for restore #99
Conversation
@@ -244,15 +244,18 @@ lazy val restoreGCS = project | |||
librarySettings, | |||
name := s"$baseName-restore-gcs" | |||
) | |||
.dependsOn(coreRestore, coreGCS) | |||
.dependsOn(coreRestore % "compile->compile;test->test", coreGCS % "compile->compile;test->test") |
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.
This change is necessary because otherwise the restore CLI fails with
[WARN ] ManifestInfo - You are using version 10.2.7 of Akka HTTP, but it appears you (perhaps indirectly) also depend on older versions of related artifacts. You can solve this by adding an explicit dependency on version 10.2.7 of the [akka-http-spray-json] artifacts to your project. Here's a complete collection of detected artifacts: (10.1.11, [akka-http-spray-json]), (10.2.7, [akka-http, akka-http-core, akka-http-testkit, akka-http-xml, akka-parsing]). See also: https://doc.akka.io/docs/akka/current/common/binary-compatibility-rules.html#mixed-versioning-is-not-allowed
Note that this is the exact same as the backup equivalent at https://github.com/aiven/guardian-for-apache-kafka/blob/main/build.sbt#L166-L173 which had the same problem.
|
||
lazy val cliRestore = project | ||
.in(file("cli-restore")) | ||
.settings( | ||
cliSettings, | ||
name := s"$baseName-cli-restore" | ||
) | ||
.dependsOn(coreCli, restoreS3, restoreGCS) | ||
.dependsOn(coreCli % "compile->compile;test->test", |
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.
These settings are also the same as the backup CLI at https://github.com/aiven/guardian-for-apache-kafka/blob/main/build.sbt#L175-L185
bff7c7f
to
5fc84c0
Compare
5fc84c0
to
acb83f3
Compare
About this change - What it does
This PR implements CLI for the restore portion of guardian.
Why this way
The structure of the PR is pretty much the same as the earlier one that implemented CLI for backup (see #83) with some additional notes/exceptions
CliSpec
tests for restore and the already existing backup. This is necessary because theMain
part ofRestore
will throw an exception when initialized because not all of the data passed into main test actually exists (i.e.backup-bucket
doesn't actually exist in S3). Note that the purpose of these tests is to see if the command line arguments arebeing correctly passed into the application's configuration, its NOT testing that the application actually runs correctly with
those arguments (thats for proper end2end tests which exist elsewhere)
try/catch
clause is not necessary for the backup because it happens to gracefully shutdown without an exception however this is an internal detail and so I added thetry/catch
clause to backup for consistency reasonsExpected an App to be initialized
. This change was done both for new Restore and current Backup. Without this change the tests wouldn't actually fail in this case as expected.override-topics
argument consists of a list of colon (:
) delimited key/values. This is because there is no standard way to parse map/dict like structures as command line arguments.:
Was picked because its not a valid character that can be used in kafka topics (see https://www.ibm.com/docs/en/integration-bus/10.0?topic=bus-producing-messages-kafka-topics). As a bonus it aesthetically looks nice.single-message-per-kafka-request
has been added. This cli flag configures the Kafka producer with a set of configuration's whos semantics allow exactly once message delivery semantics without having to use Kafka transaction API (see Support Transactional API for Restore #98).single-message-per-kafka-request
can possibly change but the user can just rely on the fact thatsingle-message-per-kafka-request
provides these semanticssingle-message-per-kafka-request
as long as it doesn't containtransactional
so its not confused with the official Kafka transactional API.RestoreClientInterface
/RestoreClient
was adjusted to accommodate aKillSwitch
. UnlikeBackup
which gives aControl
(that is gotten from the Alpakka Kafka consumer), theAlpakka
producer has no such mechanism so we use a standard akka-stream killswitch which will gracefully terminate the entire stream.Main
waits forever when callingrestore.run()
withAwait.result(app.run(), Duration.Inf)
. I did it initially this way because restore's can take significant amounts of time (theoretically even days?) and so I didn't want to add an upper bound on how long it takes because in doing so you kind of defeat the point of having an upper bound anyways.