-
Notifications
You must be signed in to change notification settings - Fork 1k
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
feat: new command to restore ksqlDB command topic backups #6361
Conversation
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.
What should we do in the case that the restore is interrupted/fails in the middle of the process? We'll have a command topic with only a partial set of commands. I'm wondering if we should just delete this incomplete command topic.
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.
I think we might actually need to use transactions to restore the command topic.
When the command topic is produced to normally, there are transactional messages produced to the command topic that take up offsets. If we restore without using transactions, this could mean the restored command topic will have a lower offset than the original command topic, that could result in query id's conflicting since we generate query id based on the offset.
For example, there are 10 command records in command topic (offset would be 15-20 potentially)
After restoring 10 command records using this tool, the offset would be 10.
We don't have to go through validation/wait for the CommandRunner like we do in DistributingExecutor so the protocol here can be much simpler.
init transaction
for each command in the backupfile
begin transaction
enqueue command
commit transaction
close producer
@@ -431,6 +431,7 @@ public void shouldBuildAggregatorParamsCorrectlyForUnwindowedAggregate() { | |||
} | |||
|
|||
@Test | |||
@SuppressWarnings("RV_RETURN_VALUE_IGNORED_NO_SIDE_EFFECT") |
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.
nit: we shouldn't check these in if this is a specific checkstyle error to your local environment
20b7cc5
to
a6dfc98
Compare
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.
LGTM, would it be possible to also add an integration test as part of this pr? We could start up a server with backups enabled, issue a bunch of DDL statements, stop server, delete command topic, run the executable to restore, start up new server and verify the metastore matches what it was previously.
...est-app/src/test/java/io/confluent/ksql/rest/server/restore/KsqlRestoreCommandTopicTest.java
Show resolved
Hide resolved
ksqldb-rest-app/src/main/java/io/confluent/ksql/rest/server/BackupReplayFile.java
Outdated
Show resolved
Hide resolved
97f8959
to
d443f43
Compare
Added an integration tests |
33d6831
to
75cc991
Compare
75cc991
to
4db06da
Compare
Description
What behavior do you want to change, why, how does your patch achieve the changes?
Fixes #6286
This is a new command that helps to restore command topic backups:
With default options, the command prompts the user to answer if continuing with the restore process or not. This is required to avoid an accidental restore process (the option -y will assume "yes" as the answer).
The restoration process displays timing information in the console to let the user know how much time it took to restore the command topic:
Testing done
Describe the testing strategy. Unit and integration tests are expected for any behavior changes.
Added unit tests
Verified manually
Reviewer checklist