fix: Remove delete.topic.enable check #3089
Conversation
TopicDeletionDisabledException is used to detect if Kafka has the delete.topic.enable config set to True. If true, then deletion is ignored by Cluster Terminate, but an error is displayed when using DROP STREAM DELETE TOPIC
Retrying a DROP STREAM DELETE TOPIC commands are delayed when retrying the failed command. This is not necessary when the TopicDeletionDisabledException is False, and even for any other error. Users can retry the command manually if it fails.
spena
force-pushed
the
remove_delete_topic_enable
branch
from
008744c
to
3ee5ee8
Compare
There was a problem hiding this comment.
Thanks @spena the change makes sense, but the code could be cleaner by avoiding the 'log and throw' antipattern. (See below).
If you make this change and remove the now superfluous comments, then it LGTM.
| // instead of going through the rest of the topics to delete. | ||
| // It is now up to the caller to ignore this exception. | ||
| LOG.info("Cannot delete topics since '" + DELETE_TOPIC_ENABLE + "' is false. "); | ||
| throw new TopicDeletionDisabledException("Topic deletion is disabled. " |
There was a problem hiding this comment.
For me, I think the 'log and throw' is an anti-pattern. Either log, or throw, not both. This pattern just leads to the same error being logged multiple times.
The fact that this requires logic when catching this exception to know not to double log is, IMHO, a clear indication this could be simplified to just throwing here, and just logging where its caught.
| ExecutorUtil.executeWithRetries( | ||
| () -> topicClient.deleteTopics(ImmutableList.of(source.getKafkaTopicName())), | ||
| RetryBehaviour.ALWAYS); | ||
| topicClient.deleteTopics(ImmutableList.of(source.getKafkaTopicName())); |
There was a problem hiding this comment.
why have we removed the retries here? Are there not still cases where we can get retryable exceptions here, e.g. on network issues, that are retryable?
There was a problem hiding this comment.
There was a delay when deleting the topic if the delete.topic.enable was false. I was thinking on not retrying if a TopicDeletionDisabledException was thrown, but I noticed that we weren't using this retry mechanism on the TopicCreateInjector, so thought this was just a copy/paste from the ClusterTerminator class.
However, I just noticed that this retry method is called in the KafkaTopicClientImpl instead. I will return it back, or better I will try to call it from the deleteTopics instead so we don't have to call retry anywhere else.
| // An exception due to TopicDeletionDisabledException should not be logged as an error, just | ||
| // info level should be enough. However, the deleteTopics() method already logged it for us, | ||
| // so we can skip it. | ||
| if (!(e instanceof TopicDeletionDisabledException)) { |
There was a problem hiding this comment.
nasty! We can clean this up with we don't log and throw, but just throw in deleteTopics.
| // An exception due to TopicDeletionDisabledException should be ignored when a Cluster | ||
| // termination is requested. The deleteTopics already logs an INFO message. We can skip | ||
| // logging it here. | ||
| if (!(e instanceof TopicDeletionDisabledException)) { |
There was a problem hiding this comment.
Again, nasty - clean up by not logging in deleteTopics.
spena
force-pushed
the
remove_delete_topic_enable
branch
from
1dfb916
to
3fb30cb
Compare
Description
The
delete.topic.enableconfiguration check done during theDROP STREAM DELETE TOPICcommand is unnecessary. This check is hiding the real error if a user cannot delete the topic when dropping the stream. Actually, the drop stream delete topic does not fail if the flag is set to False.To fix the problem, I removed the
delete.topic.enablecheck, and rely on theTopicDeletionDisableExceptionthrown when attempting to delete the topic. ForDROP STREAM DELETE TOPIC, an error will be displayed in the console about the delete.topic.enable.For Cluster termination, the exception is ignored, and let KSQL continue with the cluster termination and cleanup of any other internal state.
Testing done
Added a few test cases to validate the changes.
Verify the
DROP STREAM DELETE TOPICworks as expected with and without thedelete.topic.enableconfiguration.Reviewer checklist