-
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
Add topic retention config to WITH clause #9223
Add topic retention config to WITH clause #9223
Conversation
- Introduces a new config RETENTION_MS which can be set in the WITH clause - The RETENTION_MS config is passed to the Admin client along with existing params such as PARTIONS, REPLICAS, etc while creating the underlying topic - Currently, there is a way to set the RETENTION of the underlying topic using the WINDOW config. The RETENTION_MS does not override that param as users may be used to the WINDOW config.
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.
Implementation looks good to me, some feature level comments inline. Two other open questions:
- Can you test to make sure that this doesn't affect the retention of internal topics? (It shouldn't, but would be good to double check)
- In the case that the topic exists and retention is different (e.g. in the
CS
case), do we want to consider either (a) failing the statement or (b) allowing it to override? In the case of partitions, if you specify something different than the existing partitions the statement will fail (IIRC)
ksqldb-common/src/main/java/io/confluent/ksql/properties/with/CommonCreateConfigs.java
Show resolved
Hide resolved
ksqldb-engine/src/main/java/io/confluent/ksql/topic/TopicCreateInjector.java
Outdated
Show resolved
Hide resolved
For 2, I think we should fail if the retention specified is different than the existing topic's retention. The same is true for partitions. |
Let's make the behavior for retention consistent with the behavior for partition count. |
- Validate retention config and fail if it doesn't match existing topics - Exception includes topics backed by table which has a cleanup.policy=compact In such cases, we do not set the retention explicitly
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! Did a quick skim and have to inline comments.
ksqldb-engine/src/test/java/io/confluent/ksql/services/FakeKafkaTopicClient.java
Outdated
Show resolved
Hide resolved
ksqldb-engine/src/main/java/io/confluent/ksql/topic/TopicCreateInjector.java
Outdated
Show resolved
Hide resolved
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.
Thanks @bvarghese1
Description
Pass retention_ms as a config in the WITH clause.
This param will be used to set the topic retention while creating the topic if the topic is not already present.
It does not update the retention if the topic is already present.
Additionally, validation is performed to ensure retention_ms config cannot be changed once a topic exists similar to partition count.
Testing done
Unit tests
Integration Tests
Manual Tests using CLI
Reviewer checklist