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

feat: add constraint to deny drop sources referenced by other CREATE_AS sources #6545

Merged
merged 7 commits into from
Nov 5, 2020

Conversation

spena
Copy link
Member

@spena spena commented Oct 29, 2020

Description

This is partial implementation of feat: terminate persistent query on DROP command

The first commit of this PR was already reviewed in the above PR (#6143). This PR only adds extra work to:

  • Return DROP source constraints from DESCRIBE EXTENDED
  • Drop sources in order for Rest QTT cleanup methods

The change for this PR adds a constraint to a source to not delete it if it is referenced by another source. For instance:

ksql> create stream t1(id int) with (kafka_topic='t1');
ksql> create stream s1 as select * from t1;
ksql> terminate CSAS_S1_9;

ksql> drop stream t1;
Cannot drop T1.
The following streams and/or tables read from this source: [S1].
You need to drop them before dropping T1.

ksql> describe extended t1;
...
Sources that have a DROP constraint on this source
--------------------------------------------------
S1
...

Even if queries are terminated, the stream or table cannot be deleted until all sources that reference to it are deleted. This is a common approach in other DBs, which could keep constraints and references on tables thus avoiding deleting them leaving tables with an invalid link.

This feature was asked by #6143. The main reason of splitting the PR is for easy review the rest of the commits.

Commit #1 is already reviewed and approved. If you want you can review again, but you can opt-out, and review only Commit #2 and Commit #3, which add the list to the DESCRIBE EXTENDED and fixes the RQTT tests. The rest of the commits are just fixing a couple of tests.

Testing done

Describe the testing strategy. Unit and integration tests are expected for any behavior changes.

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

@spena spena added this to the 0.15.0 milestone Oct 29, 2020
@spena spena requested a review from a team October 29, 2020 19:51
Copy link
Contributor

@agavra agavra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@@ -68,7 +69,9 @@ public SourceDescription(
@JsonProperty("partitions") final int partitions,
@JsonProperty("replication") final int replication,
@JsonProperty("statement") final String statement,
@JsonProperty("queryOffsetSummaries") final List<QueryOffsetSummary> queryOffsetSummaries) {
@JsonProperty("queryOffsetSummaries") final List<QueryOffsetSummary> queryOffsetSummaries,
@JsonProperty("sourceConstraints") final List<String> sourceConstraints
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens if we get old response with new CLI and vice versa?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From an old CLI to the new Server works.

From the new CLI to the old Server doesn't work. But, is this a valid support? And, how do others maintain compatibility with an old Server? I was looking at the code for other properties and I don't see how this is supported. Any idea?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From an old CLI to the new Server works.

This one is more important. The other is a nice-to-have. It's not expected to work, but it would be nice if it behaved nicely (either ignoring it or telling you there's an issue). You don't need to implement that for this pr

@spena
Copy link
Member Author

spena commented Nov 4, 2020

Btw, I added a new test in RecoverTest to validate the scenario caused by an upgrade, where the DROP stream could be in an order where it has DROP constraints. There was nothing to fix. The compactor did the thing to remove the query, thus removing the DROP constraint. I'll make sure to re-validate this before merging the DROP+TERMINATE command.

@spena spena merged commit 28835a4 into confluentinc:master Nov 5, 2020
@spena spena deleted the feat_link_sources branch November 5, 2020 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants