fix: windowed tables now have cleanup policy compact+delete #5743
Conversation
| final Map<String, ?> config = | ||
| ImmutableMap.of(TopicConfig.CLEANUP_POLICY_CONFIG, topicCleanUpPolicy); |
There was a problem hiding this comment.
it seems that for create stream previously we were using the default for the cluster, but now we've changed that to explicitly specify delete. Is that correct/desired?
There was a problem hiding this comment.
The default is delete: https://kafka.apache.org/documentation/#cleanup.policy
I thought explicitly specifying the policy was cleaner than having empty vs non-empty maps, but if there are situations you're concerned about in terms of correctness we should definitely investigate / return to passing empty maps.
There was a problem hiding this comment.
Something to check is to see if the CREATE STREAM code path will change the config of a topic to delete if it already exists. Another thing to consider if it's possible (though I wouldn't understand why anyone would do this) to change the default for the cluster for cleanup.policy. If neither of these are a real concern, then I'm happy making it explicit
There was a problem hiding this comment.
Turns out it's possible to change the default cleanup.policy using the broker config log.cleanup.policy. I've switched this PR back to specifying empty configs in the case of streams in order to avoid a breaking change, but this behavior (using the default for streams and forcing tables to be compacted) feels weird to me. Is it preferable to use the broker default for streams, rather than specifying delete?
There was a problem hiding this comment.
Good question, there's definitely a strong argument toward forcing delete for streams... I could be convinced either way, perhaps now I'm actually leaning a little bit toward your original code.
There was a problem hiding this comment.
OK, I'll go with the original then.
Regarding your other question:
Something to check is to see if the CREATE STREAM code path will change the config of a topic to delete if it already exists.
This never happens since the KafkaTopicClient does not update topics if they exist. (It also doesn't even validate the cleanup policy, only number of replicas and partitions:
)…tinc#5743) BREAKING CHANGE: ksqlDB now creates windowed tables with cleanup policy "compact,delete", rather than "compact". Also, topics that back streams are always created with cleanup policy "delete", rather than the broker default (by default, "delete").
Description
Fixes #5554
BREAKING CHANGE: ksqlDB now creates windowed tables with cleanup policy "compact,delete", rather than "compact". Also, topics that back streams are always created with cleanup policy "delete", rather than the broker default (by default, "delete").
Testing done
Unit tests + manual.
Reviewer checklist