Skip to content
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

Merged
merged 1 commit into from
Feb 12, 2022
Merged

Implement CLI for restore #99

merged 1 commit into from
Feb 12, 2022

Conversation

mdedetrich
Copy link
Contributor

@mdedetrich mdedetrich commented Feb 11, 2022

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

  • A
    try backup.Main.main(args.toArray)
    catch {
      case _: Throwable =>
    }
    was added to the CliSpec tests for restore and the already existing backup. This is necessary because the Main part of
    Restore 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 are
    being 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)
    • Technically speaking the 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 the try/catch clause to backup for consistency reasons
  • If for whatever reason the configuration isn't initialized in the tests we fail with Expected 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.
  • The 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.
  • In addition to the typical command line configuration parameters 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).
    • It is intended that the underlying configurations of single-message-per-kafka-request can possibly change but the user can just rely on the fact that single-message-per-kafka-request provides these semantics
    • When Kafka transactional API is supported this will be under a different CLI flag (since to use the transactional API properly you also need to setup the consumer in a specific way which is outside the control of the restore app, the restore app only contains a publisher and not a consumer).
    • I am open to using a different name than single-message-per-kafka-request as long as it doesn't contain transactional so its not confused with the official Kafka transactional API.
  • RestoreClientInterface/RestoreClient was adjusted to accommodate a KillSwitch. Unlike Backup which gives a Control (that is gotten from the Alpakka Kafka consumer), the Alpakka producer has no such mechanism so we use a standard akka-stream killswitch which will gracefully terminate the entire stream.
  • The cli Main waits forever when calling restore.run() with Await.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.

@@ -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")
Copy link
Contributor Author

@mdedetrich mdedetrich Feb 11, 2022

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",
Copy link
Contributor Author

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

@mdedetrich mdedetrich requested review from jlprat and reta February 11, 2022 05:40
@mdedetrich mdedetrich force-pushed the implement-cli-restore branch from bff7c7f to 5fc84c0 Compare February 11, 2022 06:19
@mdedetrich mdedetrich force-pushed the implement-cli-restore branch from 5fc84c0 to acb83f3 Compare February 11, 2022 23:58
@mdedetrich mdedetrich merged commit c100e87 into main Feb 12, 2022
@mdedetrich mdedetrich deleted the implement-cli-restore branch February 12, 2022 00:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants