feat: new and initialize commands for migration tool #6988

jzaralim · 2021-02-10T17:03:11Z

Description

new: creates a new directory and empty properties file
initialize: creates migration stream and table

Reviewer checklist

Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
Ensure relevant issues are linked (description should include text like "Fixes #")

vcrfxia

Thanks @jzaralim -- awesome stuff!! Comments inline ; nothing major.

ksqldb-tools/pom.xml

ksqldb-tools/src/main/java/io/confluent/ksql/tools/migrations/MigrationConfig.java

...ls/src/main/java/io/confluent/ksql/tools/migrations/commands/InitializeMigrationCommand.java

vcrfxia · 2021-02-10T17:50:30Z

...ls/src/main/java/io/confluent/ksql/tools/migrations/commands/InitializeMigrationCommand.java

+      System.out.println(eventStreamCommand);
+      System.out.println(versionTableCommand);
+    } else {
+      final KsqlRestClient client = KsqlRestClient.create(


Any reason we can't use the actual Java client here? Unless we can think of downsides, it'd be great to dogfood. Plus we'd like to eventually migrate the CLI to use the Java client (and away from KsqlRestClient) as well.

Done, but for now KsqlRestClient is used in MigrationConfig to query the /info endpoint. This is temporary until the Java client supports this functionality.

...ls/src/main/java/io/confluent/ksql/tools/migrations/commands/InitializeMigrationCommand.java

ksqldb-tools/src/main/java/io/confluent/ksql/tools/migrations/commands/NewMigrationCommand.java

vcrfxia · 2021-02-10T17:54:15Z

ksqldb-tools/src/main/java/io/confluent/ksql/tools/migrations/commands/NewMigrationCommand.java

+      Files.createDirectories(Paths.get(projectPath));
+      new File(projectPath + "/ksql-migrations.properties").createNewFile();
+    } catch (IOException e) {
+      e.printStackTrace();


Doesn't have to be in this PR, but we should make sure we sanity-check the error messages users get if the tool doesn't have permissions to create the files/directories, or if file/directories already exist. We should also sanity check the tool's behavior (do we overwrite existing files, etc).

+1 not only that, we should return correct error codes (using System.exit) if necessary so that it can be properly piped into scripts. I think getting error messages correct is a huge part of making this tool successful.

cc @colinhicks @spena for their opinion on how this should work.

I'd also recommend avoiding error stacks in the output that users see (logs can have the error messages). This turns people off from these tools and makes them look very rough around the edges.

Yes, very important to return the correct error codes from the process. This is relevant for CI, too, where a non-zero code is expected to signal job step failure. We want to avoid forcing users to otherwise detect failure by parsing the output.

+1. I also usually avoid putting everything into one try/catch when I want to know what failed. This way I can display something like:

$ ksql-migrations new /unknown-dir/migrations-project Creating directory: <project-dir> Failed creating directory: "Directory /unknown-dir does not exist"

Which then you can return know what error code to return, like System.exit(2). This number will let user know what's wrong when writing scripts.

When working with user tools, it is always recommended to display a good error message and an error status. They will thank you a lot if they write their on bash scripts to automate this tool.

Done. For now, they all have the same error code, but after more parts of the tool are complete we can go through the errors and categorize them.

vcrfxia · 2021-02-10T17:57:04Z

...ls/src/main/java/io/confluent/ksql/tools/migrations/commands/InitializeMigrationCommand.java

+        System.out.println("Successfully created migration stream");
+      } else {
+        System.out.println("Failed to create migration stream: "
+            + streamResponse.getErrorMessage());


We should sanity-check the error message the user gets if they try calling initialize twice (i.e., the stream already exists). It may make sense to check for the existence of the stream/table ahead of issuing the create calls and print a more targeted error message in that case. The targeted error message could mention the clean command which can be used if the user wants to start over with migrations. (This command could also help get them out of situations where for whatever reason the stream was created but the table wasn't.)

...ls/src/main/java/io/confluent/ksql/tools/migrations/commands/InitializeMigrationCommand.java

ksqldb-tools/src/main/java/io/confluent/ksql/tools/migrations/MigrationConfig.java

spena · 2021-02-11T16:24:46Z

...ls/src/main/java/io/confluent/ksql/tools/migrations/commands/InitializeMigrationCommand.java

+      System.out.println(eventStreamCommand);
+      System.out.println(versionTableCommand);
+    } else {
+      final KsqlRestClient client = KsqlRestClient.create(


...ls/src/main/java/io/confluent/ksql/tools/migrations/commands/InitializeMigrationCommand.java

spena · 2021-02-11T16:48:33Z

...ls/src/main/java/io/confluent/ksql/tools/migrations/commands/InitializeMigrationCommand.java

+        System.out.println("Successfully created migration stream");
+      } else {
+        System.out.println("Failed to create migration stream: "
+            + streamResponse.getErrorMessage());


ksqldb-tools/src/main/java/io/confluent/ksql/tools/migrations/commands/NewMigrationCommand.java

spena · 2021-02-11T16:53:37Z

ksqldb-tools/src/main/java/io/confluent/ksql/tools/migrations/commands/NewMigrationCommand.java

-    throw new UnsupportedOperationException();
+    try {
+      Files.createDirectories(Paths.get(projectPath));
+      new File(projectPath + "/ksql-migrations.properties").createNewFile();


The new config should have some default properties, and perhaps commented properties names in case the user wants to customize it.

+1 but I was going to suggest this as a follow-up. I think we should update the new command to take in the ksqlDB server URL, which we can then pre-populate into the config file. This is the only required config in the config file, so we definitely want it.

I don't feel strongly about whether we populate some of the other configs with defaults or not. Could be nice for some of the ones we expect users are more likely to want to change.

Agreed, but I'll leave this as a follow-up

spena · 2021-02-11T16:58:51Z

ksqldb-tools/src/main/java/io/confluent/ksql/tools/migrations/commands/NewMigrationCommand.java

+      Files.createDirectories(Paths.get(projectPath));
+      new File(projectPath + "/ksql-migrations.properties").createNewFile();
+    } catch (IOException e) {
+      e.printStackTrace();


+1. I also usually avoid putting everything into one try/catch when I want to know what failed. This way I can display something like:

$ ksql-migrations new /unknown-dir/migrations-project Creating directory: <project-dir> Failed creating directory: "Directory /unknown-dir does not exist"

Which then you can return know what error code to return, like System.exit(2). This number will let user know what's wrong when writing scripts.

When working with user tools, it is always recommended to display a good error message and an error status. They will thank you a lot if they write their on bash scripts to automate this tool.

ksqldb-tools/src/main/java/io/confluent/ksql/tools/migrations/commands/NewMigrationCommand.java

vcrfxia

Thanks @jzaralim -- looking great! A few suggestions and minor fixes inline. Feel free to address as follow-ups. LGTM assuming everything gets addressed.

ksqldb-tools/src/main/java/io/confluent/ksql/tools/migrations/MigrationConfig.java

vcrfxia · 2021-02-16T05:38:16Z

ksqldb-tools/src/main/java/io/confluent/ksql/tools/migrations/MigrationConfig.java

+    client.close();
+
+    if (!response.isSuccessful()) {
+      LOGGER.error("Failed to query " + MigrationConfig.KSQL_SERVER_URL + "/info");


Don't worry about this for now since this code is going to be replaced with Java client code anyway, but after the replacement we should verify that this error message is actionable. For example, the current error message doesn't give any information regarding why the failure occurred (auth issue, transient network issue, invalid address, etc).

vcrfxia · 2021-02-16T05:39:16Z

...ls/src/main/java/io/confluent/ksql/tools/migrations/commands/InitializeMigrationCommand.java

+
+  @Override
+  public void run() {
+    final long startTime = System.currentTimeMillis();


We should refactor this time-tracking into BaseCommand since it's common to all commands. Feel free to postpone to a follow-up if you prefer.

vcrfxia · 2021-02-16T05:39:57Z

...ls/src/main/java/io/confluent/ksql/tools/migrations/commands/InitializeMigrationCommand.java

+        .setHost(url.getHost())
+        .setPort(url.getPort());
+
+    final Client ksqlClient = Client.create(options);


Creating the Java client from the MigrationsConfig should be factored out into a util file, since many more commands will need this. Feel free to defer to a follow-up PR.

vcrfxia · 2021-02-16T05:43:05Z

ksqldb-tools/src/main/java/io/confluent/ksql/tools/migrations/commands/NewMigrationCommand.java

+  private boolean tryCreatePropertiesFile(final String path) {
+    final File configFile = new File(path);
+    if (configFile.exists()) {
+      LOGGER.info(path + " already exists. Skipping file creation.");


This log message should at least be a warning. It might make sense to even fail the command if the config file already exists, since it doesn't make sense for the user to run migrations new in this case, especially when we introduce a new param to specify the server URL as part of the command. (cc @colinhicks for a minor product question, in case he has input)

Shouldn't all these messages go to the command output? If the command fails, it just exits with System.exit(1), and nothing shows the user what was wrong unless they go to the log.

changed to warn for now, might rethink this after adding the new param

ksqldb-tools/src/main/java/io/confluent/ksql/tools/migrations/commands/NewMigrationCommand.java

vcrfxia · 2021-02-16T05:45:05Z

ksqldb-tools/src/main/java/io/confluent/ksql/tools/migrations/MigrationConfig.java

+  public static final String KSQL_MIGRATIONS_TOPIC_REPLICAS = "ksql.migrations.topic.replicas";
+  public static final int KSQL_MIGRATIONS_TOPIC_REPLICAS_DEFAULT = 1;
+
+  private static final Logger LOGGER = LoggerFactory.getLogger(MigrationConfig.class);


What log4j config is being used with these loggers by default? Do we need to provide one with the tool?

Added a log4j config file that defaults to stdout

ksqldb-tools/pom.xml

spena · 2021-02-16T18:01:47Z

ksqldb-tools/src/main/java/io/confluent/ksql/tools/migrations/commands/NewMigrationCommand.java

+    if (directory.exists() && directory.isDirectory()) {
+      LOGGER.info(path + " already exists. Skipping directory creation.");
+      return true;
+    } else if (directory.exists() && !directory.isDirectory()) {
+      LOGGER.error(path + " already exists as a file. Cannot create directory.");
+      return false;
+    }
+
+    try {
+      LOGGER.info("Creating directory: " + path);
+      Files.createDirectories(Paths.get(path));
+    } catch(FileSystemException e) {
+      LOGGER.error("Permission denied: create directory " + path);
+      return false;
+    } catch (IOException e) {
+      LOGGER.error(String.format("Failed to create directory %s: %s", path, e.getMessage()));
+      return false;
+    }


Shouldn't all these messages go to the command output? If an error occurs, the command will not show anything unless the user goes to the log.

spena · 2021-02-16T18:03:39Z

ksqldb-tools/src/main/java/io/confluent/ksql/tools/migrations/commands/NewMigrationCommand.java

+        tryCreateDirectory(projectPath + "/migrations") &&
+        tryCreatePropertiesFile(projectPath + "/ksql-migrations.properties")) {
+      final long endTime = System.currentTimeMillis();
+      LOGGER.info("Migrations project directory created successfully (execution time " +


I am confused. Why is the message sent to the logger and not the command output?

For now, the log4j config that we provide defaults to stdout. We might revisit this later.

Zara Lim added 2 commits February 10, 2021 08:56

new and initialize commands

b6785ac

add new command

425fa6f

jzaralim requested a review from a team as a code owner February 10, 2021 17:03

vcrfxia reviewed Feb 10, 2021

View reviewed changes

spena reviewed Feb 11, 2021

View reviewed changes

switch to Java client, refactor configs, use logger instead of stdout

cd8d4b7

vcrfxia approved these changes Feb 16, 2021

View reviewed changes

spena reviewed Feb 16, 2021

View reviewed changes

Zara Lim and others added 7 commits February 16, 2021 12:49

add log4j config file, other small changes

e91c108

fix version number

4f83643

fix checkstyle

f5fb85e

Merge branch 'initialize' of github.com:jzaralim/ksql into initialize

e93ea5a

final class

fa944f3

suppress dm_exit warnings

49e0f32

suppress fb warnings

0a67d6b

jzaralim merged commit 8bdb09a into confluentinc:master Feb 17, 2021

jzaralim deleted the initialize branch February 17, 2021 09:13

jzaralim mentioned this pull request Feb 17, 2021

chore: refactor schema migrations tool #7020

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: new and initialize commands for migration tool #6988

feat: new and initialize commands for migration tool #6988

jzaralim commented Feb 10, 2021

vcrfxia left a comment

vcrfxia Feb 10, 2021

colinhicks Feb 10, 2021

spena Feb 11, 2021

jzaralim Feb 11, 2021

vcrfxia Feb 10, 2021

agavra Feb 10, 2021

agavra Feb 10, 2021

colinhicks Feb 10, 2021

spena Feb 11, 2021

jzaralim Feb 11, 2021

vcrfxia Feb 10, 2021

spena Feb 11, 2021

spena Feb 11, 2021

spena Feb 11, 2021

spena Feb 11, 2021

vcrfxia Feb 11, 2021

jzaralim Feb 11, 2021

spena Feb 11, 2021

vcrfxia left a comment

vcrfxia Feb 16, 2021

vcrfxia Feb 16, 2021

vcrfxia Feb 16, 2021

vcrfxia Feb 16, 2021

spena Feb 16, 2021

jzaralim Feb 16, 2021

vcrfxia Feb 16, 2021

jzaralim Feb 16, 2021

spena Feb 16, 2021

spena Feb 16, 2021

jzaralim Feb 16, 2021

feat: new and initialize commands for migration tool #6988

feat: new and initialize commands for migration tool #6988

Conversation

jzaralim commented Feb 10, 2021

Description

Reviewer checklist

vcrfxia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vcrfxia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment