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: assert not exists topic #9086

Merged
merged 1 commit into from
May 2, 2022
Merged

feat: assert not exists topic #9086

merged 1 commit into from
May 2, 2022

Conversation

jzaralim
Copy link
Contributor

@jzaralim jzaralim commented May 1, 2022

Description

Adds NOT EXISTS to ASSERT TOPIC which asserts that a topic doesn't exist

Testing done

Manual, unit+integration tests

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

@jzaralim jzaralim marked this pull request as ready for review May 2, 2022 02:00
@jzaralim jzaralim requested a review from a team as a code owner May 2, 2022 02:00
@jzaralim jzaralim requested a review from jnh5y May 2, 2022 02:01
LOG.warn("Will skip topic config check for topic non-existence assertion.");
}
if (topicExists) {
throw new KsqlException("Topic " + topic + " exists");
Copy link
Member

Choose a reason for hiding this comment

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

I'm 95% sure, but just to make sense, but this logic is instead the call which is retried, so if the topic is being deleted, the assertion can pass, etc, etc, right? (Still waiting for my coffee to kick in.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, if this fails then the entire assertTopic method is retried until the timeout

Copy link
Member

@jnh5y jnh5y left a comment

Choose a reason for hiding this comment

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

LGTM. (Once this is in, looks like we'd just need a doc PR.)

@jzaralim jzaralim merged commit 4b57b55 into master May 2, 2022
@jzaralim jzaralim deleted the not-exist branch May 2, 2022 17:16
@jzaralim
Copy link
Contributor Author

jzaralim commented May 2, 2022

LGTM. (Once this is in, looks like we'd just need a doc PR.)

We also need to update the Java client and the migration tool, and we need to do all this again for ASSERT SCHEMA :)

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.

2 participants