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

fix: check for other sources using a topic before deleting #3070

Merged
merged 1 commit into from
Jul 23, 2019

Conversation

rodesai
Copy link
Contributor

@rodesai rodesai commented Jul 12, 2019

When handling DELETE TOPIC, check that other sources don't use the topic
before deleting. This is an optimistic check - the user could still delete
topics out from under us, and its not safe racing against concurrent statements.
But, we've seen single users kick themselves this way in the past, so this should
help in those cases.

@rodesai rodesai requested a review from a team as a code owner July 12, 2019 04:53
Copy link
Contributor

@big-andy-coates big-andy-coates left a comment

Choose a reason for hiding this comment

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

LGTM @rodesai

@@ -75,6 +78,26 @@ public TopicDeleteInjector(
Objects.requireNonNull(schemaRegistryClient, "schemaRegistryClient");
}

private void checkTopicRefs(final DataSource<?> source) {
Copy link
Contributor

Choose a reason for hiding this comment

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

super nit: private methods to the bottom of class files, not interspersed with the public API.

.filter(s -> s.getKafkaTopicName().equals(topicName))
.map(s -> s.getName())
.filter(name -> !sourceName.equals(name))
.collect(Collectors.toList());
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than create a list, you can just build the string you need later:

final String using = sources.values().stream()
        .filter(s -> s.getKafkaTopicName().equals(topicName))
        .map(s -> s.getName())
        .filter(name -> !sourceName.equals(name))
        .collect(Collectors.joining(", "));

if (!using.isEmpty()) {
      throw new KsqlException(
          String.format(
              "Refusing to delete topic. Found other data sources (%s) using topic %s",
             using,
              topicName
          )
      );
    }

@big-andy-coates big-andy-coates requested review from a team, vcrfxia, hjafarpour, agavra and spena and removed request for a team July 12, 2019 15:34
Copy link
Contributor

@hjafarpour hjafarpour left a comment

Choose a reason for hiding this comment

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

LGTM.

When handling DELETE TOPIC, check that other sources don't use the topic
before deleting. This is an optimistic check - the user could still delete
topics out from under us, and its not safe racing against concurrent statements.
But, we've seen single users kick themselves this way in the past, so this should
help in those cases.
@rodesai rodesai force-pushed the check-topic-refs-before-delete branch from 4da9686 to aa9319e Compare July 14, 2019 19:08
@rodesai rodesai merged commit b3fa315 into confluentinc:master Jul 23, 2019
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.

3 participants