-
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
fix: clean up custom prefixed internal topics #8640
fix: clean up custom prefixed internal topics #8640
Conversation
ksqldb-engine/src/main/java/io/confluent/ksql/engine/QueryCleanupService.java
Outdated
Show resolved
Hide resolved
ksqldb-engine/src/main/java/io/confluent/ksql/engine/QueryCleanupService.java
Outdated
Show resolved
Hide resolved
ksqldb-engine/src/main/java/io/confluent/ksql/services/KafkaTopicClientImpl.java
Show resolved
Hide resolved
Hey, this is actually related to #8607 but I only happened to notice it just now -- I think this should (a) be factored into a public utility method so that we have a single source of truth if we ever need to access it elsewhere rather than try to rebuild it each time and trying to keep everything in sync -- I'd recommend adding it to QueryApplicationId, yes it's not technically an app id but it's essentially analogue to one and anyways shares most of the same components plus we'd presumably want to stay in sync if e.g. we ever added yet another component to the query app id (like a new prefix config, idk 🤷♀️ ) and (b) consider making it completely identical to the app id for shared runtimes, just dropping the index/replacing that with the "query" thing On that note: is the only reason we add the "query" to this prefix to make it easier/possible to parse and extract the prefix substring? Because if we don't have a strict need for this "query" piece I believe we can just reuse the |
For example could we just call QueryApplicationId#build or the new utility method if we add one rather than do this parsing here? |
...db-rest-app/src/main/java/io/confluent/ksql/rest/server/restore/KsqlRestoreCommandTopic.java
Outdated
Show resolved
Hide resolved
} catch (final Exception e) { | ||
System.out.println(String.format("Failed to clean up query %s ", applicationId)); | ||
System.out.println(String.format("Failed to clean up query %s ", topicPrefix)); |
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.
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.
@ableegoldman The restore cmd topic tool is run via the cli I imagine that it is used there
I'm +100 on this. Otherwise, the PR lgtm. |
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.
Looking good, had some code clarity suggestions that I think are worth committing since the PR build hasn't been going for very long yet
Beyond that I'd recommend running all the tests for any module that's touched on in this PR or seems potentially connected, if you aren't already. Don't want to have to wait for the entire PR build to run to discover there's some failing unit test or checkstyle issue, let's get this guy merged ASAP!
ksqldb-engine/src/main/java/io/confluent/ksql/engine/QueryCleanupService.java
Outdated
Show resolved
Hide resolved
@@ -30,7 +30,7 @@ public static String buildSharedRuntimeId( | |||
final boolean persistent, | |||
final int sharedRuntimeIndex | |||
) { | |||
final String queryAppId = buildPrefix(config, persistent) + "-" + sharedRuntimeIndex; |
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.
seriously great catch -- would have sucked to get stuck with this awkward looking thing
ksqldb-common/src/main/java/io/confluent/ksql/util/QueryApplicationId.java
Outdated
Show resolved
Hide resolved
The test failure should be fixed by #8664 |
#8607 made it so the app id was not always the prefix for internal topics now we clean up based on that not the app id
Testing done
Describe the testing strategy. Unit and integration tests are expected for any behavior changes.
Reviewer checklist