KAFKA-6418: AdminClient should handle empty or null topic names better #4470
Conversation
|
🖤 |
|
Test failure is |
|
Retest this please |
| if (!topicFutures.containsKey(topicName)) { | ||
| if (topicNameIsUnrepresentable(topicName)) { | ||
| KafkaFutureImpl<TopicDescription> future = new KafkaFutureImpl<TopicDescription>(); | ||
| future.completeExceptionally(new InvalidTopicException("The given topic name cannot be " + |
There was a problem hiding this comment.
May as well add topicName to the message so that the user knows which case they've hit? Same for the others.
| env.kafkaClient().prepareMetadataUpdate(env.cluster(), Collections.<String>emptySet()); | ||
| env.kafkaClient().setNode(env.cluster().controller()); | ||
|
|
||
| List<String> sillyTopicNames = new ArrayList<>(); |
There was a problem hiding this comment.
nit: Arrays.asList a bit more concise.
| @@ -215,6 +215,41 @@ public void testCreateTopics() throws Exception { | |||
| } | |||
| } | |||
|
|
|||
| @Test | |||
| public void testInvalidTopicNames() throws Exception { | |||
There was a problem hiding this comment.
nit (open to discussion): I tend to prefer smaller test cases when they are possible since it is easier to narrow down the problem. Any reason not to split this into 3 separate cases: one for each API?
There was a problem hiding this comment.
It's just more boilerplate creating each test. This particular test actually runs very fast since it's a true unit test (no brokers running), so test time should not be an issue. I don't feel strongly about it, though.
There was a problem hiding this comment.
Yeah, I don't feel too strongly in this case. The advantage generally is that the scope of the test case is clearer which makes failures easier to investigate.
|
retest this please |
AdminClient should return an InvalidTopicException when the topic name supplied by an AC user is empty or null. Previously, the client would try to serialize these invalid topic names, and the whole batch request would fail, including unrelated topics. Also add a unit test.