feat: update ksql restore command to skip incompatible commands if flag set #6524

stevenpyzhang · 2020-10-26T21:39:22Z

Description

Added a -s flag for skipping commands when restoring. If the flag isn't present, the restore command program will exit if it encounters errors deserializing commands from the command topic backup file. If it is present and true, it'll skip over the incompatible command instead. The program then prints out the number of skipped commands and tries to clean up any left over KafkaStreams internal topics and state stores if the skipped command was a query.

Testing done

Added integration test
Local manual test

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 #")

stevenpyzhang · 2020-10-26T21:40:44Z

ksqldb-rest-app/src/main/java/io/confluent/ksql/rest/server/CommandTopic.java

@@ -90,7 +90,6 @@ public void start() {
          log.warn("Backup is out of sync with the current command topic. "
              + "Backups will not work until the previous command topic is "
              + "restored or all backup files are deleted.", e);
-          return records;


We should be returning the record even if it isn't backed up. The CommandRunner has logic to detect when the command topic and backup are out of sync/the command is incompatible.

this check detects the case where the backup and the command topic are inconsistent (have different contents). in that case we shouldn't return the command here, otherwise the command runner will try to execute it.

We should still return the record in some cases. For example, the command topic backup will fail to back up incompatible commands (it tries to deserialize the command before backing it up) Currently the throwOnInvalidRecords in the CommandTopicBackupImpl doesn't count as corruption so the incompatible commands don't actually cause the server to enter degraded state if backups are enabled. I think these incompatible commands should still be returned so the CommandRunner can attempt to deserialize it and enter degraded state properly

@rodesai I changed it so that we still return the command in the case of a KsqlException since this is thrown by the backup when there's an issue deserializing either the commandId or command. The command isn't returned if it's a KsqlServerException since that means the corruption check has triggered.

ksqldb-rest-app/src/main/java/io/confluent/ksql/rest/server/restore/RestoreOptions.java

spena · 2020-10-27T13:54:41Z

...db-rest-app/src/main/java/io/confluent/ksql/rest/server/restore/KsqlRestoreCommandTopic.java

-    throwOnInvalidRecords(records);
+    if (options.isSkipIncompatibleCommands()) {
+      records = removeIncompatibleCommands(records);
+    }


should we call throwOnInvalidRecords when skip is false? That way the restore will fail if a command is invalid, thus avoiding re-creating the command topic and writing partial commands.

Ok I changed it so that we always check that the commands are valid. The only different with the skip option is that if it's set to true, instead of throwing an exception and exiting the program when failing to deserialize the command due to a SerializationException/IncomaptibleKsqlCommandVersionException (which indicates an incompatible command) the method continues by skipping the command.

...-app/src/test/java/io/confluent/ksql/api/integration/RestoreCommandTopicIntegrationTest.java

rodesai · 2020-10-28T06:06:53Z

ksqldb-rest-app/src/main/java/io/confluent/ksql/rest/server/CommandTopic.java

@@ -117,7 +116,6 @@ public void start() {
          log.warn("Backup is out of sync with the current command topic. "
              + "Backups will not work until the previous command topic is "
              + "restored or all backup files are deleted.", e);
-          return restoreCommands;


We need to make the above exception more specific. We shouldn't just ignore any exception. Ditto for the other place this is used. I know this isn't part of your patch, but might as well do this now.

I changed it to only catch a KsqlException or KsqlServerException

rodesai · 2020-10-28T06:13:09Z

ksqldb-rest-app/src/main/java/io/confluent/ksql/rest/server/CommandTopic.java

@@ -90,7 +90,6 @@ public void start() {
          log.warn("Backup is out of sync with the current command topic. "
              + "Backups will not work until the previous command topic is "
              + "restored or all backup files are deleted.", e);
-          return records;


this check detects the case where the backup and the command topic are inconsistent (have different contents). in that case we shouldn't return the command here, otherwise the command runner will try to execute it.

spena

lgmt

stevenpyzhang

This PR still needs some work actually, we need to check if the commands that are skipped have queries in them, and then clean up internal topics and state stores for them.

rodesai

Couple more nits inline around naming/exception types

rodesai · 2020-11-05T07:24:08Z

ksqldb-rest-app/src/main/java/io/confluent/ksql/rest/server/CommandTopic.java

-        } catch (final Exception e) {
+        } catch (final KsqlException e) {
+          log.warn("Error encountered while backing up command.", e);
+        } catch (final KsqlServerException e) {


we should define a new exception type for this case. this is a very specific error condition, and it should have its own exception class.

rodesai · 2020-11-05T07:30:29Z

ksqldb-rest-app/src/main/java/io/confluent/ksql/rest/server/CommandTopic.java

@@ -86,7 +89,9 @@ public void start() {
      for (ConsumerRecord<byte[], byte[]> record : iterable) {
        try {
          backupRecord(record);
-        } catch (final Exception e) {
+        } catch (final KsqlException e) {


again - this should be a very specific exception indicating a deserialization error. Also, why are we even trying to deserialize the command from the backup logic (cc @spena)? It doesn't look like we're using any of the fields.

Ok after talking it over with @spena , we don't need this deserialization logic in the backup impl, it should just backup whatever is found in the command topic, deserialization should be handled after it's backed up. So I'll get rid of this catch block and we'll only have the KsqlServerException one.

...db-rest-app/src/main/java/io/confluent/ksql/rest/server/restore/KsqlRestoreCommandTopic.java

…ag set

rodesai

LGTM!

stevenpyzhang requested a review from a team as a code owner October 26, 2020 21:39

stevenpyzhang requested a review from spena October 26, 2020 21:39

stevenpyzhang commented Oct 26, 2020

View reviewed changes

stevenpyzhang force-pushed the truncate-command-topic branch 2 times, most recently from 64a2ccc to 6c54d4a Compare October 26, 2020 21:47

spena reviewed Oct 27, 2020

View reviewed changes

stevenpyzhang force-pushed the truncate-command-topic branch from 95fa2c8 to ce2a9b5 Compare October 28, 2020 04:30

stevenpyzhang requested review from spena and a team October 28, 2020 04:32

rodesai suggested changes Oct 28, 2020

View reviewed changes

spena approved these changes Oct 28, 2020

View reviewed changes

stevenpyzhang commented Oct 28, 2020

View reviewed changes

stevenpyzhang force-pushed the truncate-command-topic branch 4 times, most recently from 5205813 to ab3b497 Compare November 3, 2020 20:22

stevenpyzhang requested review from rodesai and a team November 3, 2020 21:41

stevenpyzhang force-pushed the truncate-command-topic branch 2 times, most recently from 29202f2 to 507fe96 Compare November 4, 2020 19:15

rodesai reviewed Nov 5, 2020

View reviewed changes

stevenpyzhang requested a review from rodesai November 5, 2020 18:08

stevenpyzhang added 5 commits November 6, 2020 12:33

feat: update ksql restore command to skip incompatible commands if fl…

edd76eb

…ag set

comments

3d95497

cleanup streams internal topics and state store

f4fcf11

remove deserialization from backup

7e7f4ae

add specific exception for corruption, also fixed a typo

d0a5d16

stevenpyzhang force-pushed the truncate-command-topic branch from 75a4ec6 to d0a5d16 Compare November 6, 2020 20:33

rodesai approved these changes Nov 12, 2020

View reviewed changes

stevenpyzhang merged commit 4d0c997 into confluentinc:master Nov 12, 2020

stevenpyzhang mentioned this pull request Jan 15, 2021

test: add integration test for incompatible command detection with backups enabled #6870

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: update ksql restore command to skip incompatible commands if flag set #6524

feat: update ksql restore command to skip incompatible commands if flag set #6524

stevenpyzhang commented Oct 26, 2020 •

edited

Loading

stevenpyzhang Oct 26, 2020

rodesai Oct 28, 2020

stevenpyzhang Oct 28, 2020 •

edited

Loading

stevenpyzhang Nov 3, 2020

spena Oct 27, 2020

stevenpyzhang Oct 28, 2020

rodesai Oct 28, 2020

stevenpyzhang Nov 3, 2020

rodesai Oct 28, 2020

spena left a comment

stevenpyzhang left a comment

rodesai left a comment

rodesai Nov 5, 2020

rodesai Nov 5, 2020

stevenpyzhang Nov 5, 2020

rodesai left a comment

feat: update ksql restore command to skip incompatible commands if flag set #6524

feat: update ksql restore command to skip incompatible commands if flag set #6524

Conversation

stevenpyzhang commented Oct 26, 2020 • edited Loading

Description

Testing done

Reviewer checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenpyzhang Oct 28, 2020 • edited Loading

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

spena left a comment

Choose a reason for hiding this comment

stevenpyzhang left a comment

Choose a reason for hiding this comment

rodesai 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

rodesai left a comment

Choose a reason for hiding this comment

stevenpyzhang commented Oct 26, 2020 •

edited

Loading

stevenpyzhang Oct 28, 2020 •

edited

Loading