You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2021/03/18 16:29:06 UTC
[GitHub] [kafka] jolshan opened a new pull request #10351: MINOR: use new method to get number of topics in DeleteTopicsRequest
jolshan opened a new pull request #10351:
URL: https://github.com/apache/kafka/pull/10351
As a result of https://github.com/apache/kafka/pull/9684, a new field for topic names was created. For versions 6+ `DeleteTopicsRequestData.topicNames` will return an empty list in KafkaApis. This PR uses a new method to efficiently initialize the size of the collection.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on a change in pull request #10351: MINOR: use new method to get number of topics in DeleteTopicsRequest
Posted by GitBox <gi...@apache.org>.
dajac commented on a change in pull request #10351:
URL: https://github.com/apache/kafka/pull/10351#discussion_r597859840
##########
File path: clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java
##########
@@ -930,6 +930,28 @@ public void testCreateTopicRequestV3FailsIfNoPartitionsOrReplicas() {
assertTrue(exception.getMessage().contains("[foo, bar]"));
}
+ @Test
+ public void testDeleteTopicsRequestNumTopics() {
+ for (short version : DELETE_TOPICS.allVersions()) {
+ DeleteTopicsRequest request = createDeleteTopicsRequest(version);
+ DeleteTopicsRequest serializedRequest = DeleteTopicsRequest.parse(request.serialize(), version);
+ // createDeleteTopicsRequest sets 2 topics
+ assertEquals(2, request.numberOfTopics());
+ assertEquals(2, serializedRequest.numberOfTopics());
Review comment:
Ah, right. I completely missed that part in the builder. We usually put unit tests for requests in their own classes (e.g. DeleteTopicsRequestTest) except the ones really verifying the serialisation which are here. I wonder if this one wouldn't be better in `DeleteTopicsRequestTest`. What do you think?
Slightly related, do we have a unit test verifying the normalization logic in the builder?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jolshan commented on a change in pull request #10351: MINOR: use new method to get number of topics in DeleteTopicsRequest
Posted by GitBox <gi...@apache.org>.
jolshan commented on a change in pull request #10351:
URL: https://github.com/apache/kafka/pull/10351#discussion_r597945039
##########
File path: clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java
##########
@@ -930,6 +930,28 @@ public void testCreateTopicRequestV3FailsIfNoPartitionsOrReplicas() {
assertTrue(exception.getMessage().contains("[foo, bar]"));
}
+ @Test
+ public void testDeleteTopicsRequestNumTopics() {
+ for (short version : DELETE_TOPICS.allVersions()) {
+ DeleteTopicsRequest request = createDeleteTopicsRequest(version);
+ DeleteTopicsRequest serializedRequest = DeleteTopicsRequest.parse(request.serialize(), version);
+ // createDeleteTopicsRequest sets 2 topics
+ assertEquals(2, request.numberOfTopics());
+ assertEquals(2, serializedRequest.numberOfTopics());
Review comment:
Right. That makes sense. Will do.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jolshan commented on a change in pull request #10351: MINOR: use new method to get number of topics in DeleteTopicsRequest
Posted by GitBox <gi...@apache.org>.
jolshan commented on a change in pull request #10351:
URL: https://github.com/apache/kafka/pull/10351#discussion_r597760202
##########
File path: clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java
##########
@@ -930,6 +930,28 @@ public void testCreateTopicRequestV3FailsIfNoPartitionsOrReplicas() {
assertTrue(exception.getMessage().contains("[foo, bar]"));
}
+ @Test
+ public void testDeleteTopicsRequestNumTopics() {
+ for (short version : DELETE_TOPICS.allVersions()) {
+ DeleteTopicsRequest request = createDeleteTopicsRequest(version);
+ DeleteTopicsRequest serializedRequest = DeleteTopicsRequest.parse(request.serialize(), version);
+ // createDeleteTopicsRequest sets 2 topics
+ assertEquals(2, request.numberOfTopics());
+ assertEquals(2, serializedRequest.numberOfTopics());
Review comment:
This shouldn't fail. versions greater or equal to 6 will get the topics written to the topics field when building, and this is what numberOfTopics looks at when version is greater or equal to 6.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jolshan commented on a change in pull request #10351: MINOR: use new method to get number of topics in DeleteTopicsRequest
Posted by GitBox <gi...@apache.org>.
jolshan commented on a change in pull request #10351:
URL: https://github.com/apache/kafka/pull/10351#discussion_r597863428
##########
File path: clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java
##########
@@ -930,6 +930,28 @@ public void testCreateTopicRequestV3FailsIfNoPartitionsOrReplicas() {
assertTrue(exception.getMessage().contains("[foo, bar]"));
}
+ @Test
+ public void testDeleteTopicsRequestNumTopics() {
+ for (short version : DELETE_TOPICS.allVersions()) {
+ DeleteTopicsRequest request = createDeleteTopicsRequest(version);
+ DeleteTopicsRequest serializedRequest = DeleteTopicsRequest.parse(request.serialize(), version);
+ // createDeleteTopicsRequest sets 2 topics
+ assertEquals(2, request.numberOfTopics());
+ assertEquals(2, serializedRequest.numberOfTopics());
Review comment:
I think the reason I didn't put it in `DeleteTopicsRequestTest` is also because that is an integration test. Would it make sense to rename that and create a unit test called DeleteTopicsRequestTest?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on a change in pull request #10351: MINOR: use new method to get number of topics in DeleteTopicsRequest
Posted by GitBox <gi...@apache.org>.
dajac commented on a change in pull request #10351:
URL: https://github.com/apache/kafka/pull/10351#discussion_r598559725
##########
File path: clients/src/test/java/org/apache/kafka/common/requests/DeleteTopicsRequestTest.java
##########
@@ -0,0 +1,125 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.common.requests;
+
+import org.apache.kafka.common.Uuid;
+import org.apache.kafka.common.errors.UnsupportedVersionException;
+import org.apache.kafka.common.message.DeleteTopicsRequestData;
+import org.junit.jupiter.api.Test;
+
+import java.util.Arrays;
+import java.util.Iterator;
+import java.util.List;
+
+import static org.apache.kafka.common.protocol.ApiKeys.DELETE_TOPICS;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+public class DeleteTopicsRequestTest {
+
+ @Test
+ public void testTopicNormalization() {
+ for (short version : DELETE_TOPICS.allVersions()) {
+ // First check topic names are in the correct place when using topicNames.
+ String topic1 = "topic1";
+ String topic2 = "topic2";
+ List<String> topics = Arrays.asList(topic1, topic2);
+ DeleteTopicsRequest requestWithNames = new DeleteTopicsRequest.Builder(new DeleteTopicsRequestData()
+ .setTopicNames(topics)).build(version);
+ DeleteTopicsRequest requestWithNamesSerialized = DeleteTopicsRequest.parse(requestWithNames.serialize(), version);
+
+ assertEquals(topics, requestWithNames.topicNames());
+ assertEquals(topics, requestWithNamesSerialized.topicNames());
+
+ if (version < 6) {
+ assertEquals(topics, requestWithNames.data().topicNames());
+ assertEquals(topics, requestWithNamesSerialized.data().topicNames());
+ } else {
+ // topics in TopicNames are moved to new topics field
+ Iterator<String> topicsIterator = topics.iterator();
+ requestWithNames.data().topics().forEach(topic -> assertEquals(topicsIterator.next(), topic.name()));
Review comment:
Could we replace this with something like the following?
```
assertEquals(topics, requestWithNames.data().topics().map(DeleteTopicState::name).collect(toList));
```
It is a bit easier to read and `assertEquals` gives the differences between all the expected and the existing topics when it fails.
##########
File path: clients/src/test/java/org/apache/kafka/common/requests/DeleteTopicsRequestTest.java
##########
@@ -0,0 +1,125 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.common.requests;
+
+import org.apache.kafka.common.Uuid;
+import org.apache.kafka.common.errors.UnsupportedVersionException;
+import org.apache.kafka.common.message.DeleteTopicsRequestData;
+import org.junit.jupiter.api.Test;
+
+import java.util.Arrays;
+import java.util.Iterator;
+import java.util.List;
+
+import static org.apache.kafka.common.protocol.ApiKeys.DELETE_TOPICS;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+public class DeleteTopicsRequestTest {
+
+ @Test
+ public void testTopicNormalization() {
+ for (short version : DELETE_TOPICS.allVersions()) {
+ // First check topic names are in the correct place when using topicNames.
+ String topic1 = "topic1";
+ String topic2 = "topic2";
+ List<String> topics = Arrays.asList(topic1, topic2);
+ DeleteTopicsRequest requestWithNames = new DeleteTopicsRequest.Builder(new DeleteTopicsRequestData()
+ .setTopicNames(topics)).build(version);
+ DeleteTopicsRequest requestWithNamesSerialized = DeleteTopicsRequest.parse(requestWithNames.serialize(), version);
+
+ assertEquals(topics, requestWithNames.topicNames());
+ assertEquals(topics, requestWithNamesSerialized.topicNames());
+
+ if (version < 6) {
+ assertEquals(topics, requestWithNames.data().topicNames());
+ assertEquals(topics, requestWithNamesSerialized.data().topicNames());
+ } else {
+ // topics in TopicNames are moved to new topics field
+ Iterator<String> topicsIterator = topics.iterator();
+ requestWithNames.data().topics().forEach(topic -> assertEquals(topicsIterator.next(), topic.name()));
+ Iterator<String> topicsIterator2 = topics.iterator();
+ requestWithNames.data().topics().forEach(topic -> assertEquals(topicsIterator2.next(), topic.name()));
+ }
+
+ // Ensure we only use new topics field on versions 6+.
+ try {
+ DeleteTopicsRequest requestWithNames2 = new DeleteTopicsRequest.Builder(new DeleteTopicsRequestData()
+ .setTopics(Arrays.asList(new DeleteTopicsRequestData.DeleteTopicState().setName(topic1),
+ new DeleteTopicsRequestData.DeleteTopicState().setName(topic2)))).build(version);
+ DeleteTopicsRequest requestWithNamesSerialized2 = DeleteTopicsRequest.parse(requestWithNames2.serialize(), version);
+
+ assertTrue(version >= 6);
+ assertEquals(topics, requestWithNames2.topicNames());
+ assertEquals(topics, requestWithNamesSerialized2.topicNames());
+
+ } catch (UnsupportedVersionException e) {
+ // We should fail if version is less than 6.
+ assertTrue(version < 6);
+ }
+
+ // Next check topic IDs are handled correctly. We should only use this field on versions 6+.
+ Uuid topicId1 = Uuid.randomUuid();
+ Uuid topicId2 = Uuid.randomUuid();
+ List<Uuid> topicIds = Arrays.asList(topicId1, topicId2);
+
+ try {
+ DeleteTopicsRequest requestWithIds = new DeleteTopicsRequest.Builder(new DeleteTopicsRequestData()
+ .setTopics(Arrays.asList(new DeleteTopicsRequestData.DeleteTopicState().setTopicId(topicId1),
+ new DeleteTopicsRequestData.DeleteTopicState().setTopicId(topicId2)))).build(version);
+ DeleteTopicsRequest requestWithIdsSerialized = DeleteTopicsRequest.parse(requestWithIds.serialize(), version);
+
+ // Version should be at least 6
+ assertTrue(version >= 6);
+ assertEquals(topicIds, requestWithIds.topicIds());
+ assertEquals(topicIds, requestWithIdsSerialized.topicIds());
+
+ // All topic names should be replaced with null
+ requestWithIds.data().topics().forEach(topic -> assertNull(topic.name()));
+ requestWithIdsSerialized.data().topics().forEach(topic -> assertNull(topic.name()));
+ } catch (UnsupportedVersionException e) {
Review comment:
ditto.
##########
File path: clients/src/test/java/org/apache/kafka/common/requests/DeleteTopicsRequestTest.java
##########
@@ -0,0 +1,125 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.common.requests;
+
+import org.apache.kafka.common.Uuid;
+import org.apache.kafka.common.errors.UnsupportedVersionException;
+import org.apache.kafka.common.message.DeleteTopicsRequestData;
+import org.junit.jupiter.api.Test;
+
+import java.util.Arrays;
+import java.util.Iterator;
+import java.util.List;
+
+import static org.apache.kafka.common.protocol.ApiKeys.DELETE_TOPICS;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+public class DeleteTopicsRequestTest {
+
+ @Test
+ public void testTopicNormalization() {
+ for (short version : DELETE_TOPICS.allVersions()) {
+ // First check topic names are in the correct place when using topicNames.
+ String topic1 = "topic1";
+ String topic2 = "topic2";
+ List<String> topics = Arrays.asList(topic1, topic2);
+ DeleteTopicsRequest requestWithNames = new DeleteTopicsRequest.Builder(new DeleteTopicsRequestData()
+ .setTopicNames(topics)).build(version);
+ DeleteTopicsRequest requestWithNamesSerialized = DeleteTopicsRequest.parse(requestWithNames.serialize(), version);
+
+ assertEquals(topics, requestWithNames.topicNames());
+ assertEquals(topics, requestWithNamesSerialized.topicNames());
+
+ if (version < 6) {
+ assertEquals(topics, requestWithNames.data().topicNames());
+ assertEquals(topics, requestWithNamesSerialized.data().topicNames());
+ } else {
+ // topics in TopicNames are moved to new topics field
+ Iterator<String> topicsIterator = topics.iterator();
+ requestWithNames.data().topics().forEach(topic -> assertEquals(topicsIterator.next(), topic.name()));
+ Iterator<String> topicsIterator2 = topics.iterator();
+ requestWithNames.data().topics().forEach(topic -> assertEquals(topicsIterator2.next(), topic.name()));
+ }
+
+ // Ensure we only use new topics field on versions 6+.
+ try {
+ DeleteTopicsRequest requestWithNames2 = new DeleteTopicsRequest.Builder(new DeleteTopicsRequestData()
+ .setTopics(Arrays.asList(new DeleteTopicsRequestData.DeleteTopicState().setName(topic1),
Review comment:
nit: Could we move `new DeleteTopicsRequestData.DeleteTopicState().setName(topic1)` to a new line in order to align it with the following `new DeleteTopicsRequestData`? There are few other cases like this.
##########
File path: clients/src/test/java/org/apache/kafka/common/requests/DeleteTopicsRequestTest.java
##########
@@ -0,0 +1,125 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.common.requests;
+
+import org.apache.kafka.common.Uuid;
+import org.apache.kafka.common.errors.UnsupportedVersionException;
+import org.apache.kafka.common.message.DeleteTopicsRequestData;
+import org.junit.jupiter.api.Test;
+
+import java.util.Arrays;
+import java.util.Iterator;
+import java.util.List;
+
+import static org.apache.kafka.common.protocol.ApiKeys.DELETE_TOPICS;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+public class DeleteTopicsRequestTest {
+
+ @Test
+ public void testTopicNormalization() {
Review comment:
We test different scenarios in the unit test. I wonder if it would make sense to break it into three separate unit tests. What do you think?
##########
File path: clients/src/test/java/org/apache/kafka/common/requests/DeleteTopicsRequestTest.java
##########
@@ -0,0 +1,125 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.common.requests;
+
+import org.apache.kafka.common.Uuid;
+import org.apache.kafka.common.errors.UnsupportedVersionException;
+import org.apache.kafka.common.message.DeleteTopicsRequestData;
+import org.junit.jupiter.api.Test;
+
+import java.util.Arrays;
+import java.util.Iterator;
+import java.util.List;
+
+import static org.apache.kafka.common.protocol.ApiKeys.DELETE_TOPICS;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+public class DeleteTopicsRequestTest {
+
+ @Test
+ public void testTopicNormalization() {
+ for (short version : DELETE_TOPICS.allVersions()) {
+ // First check topic names are in the correct place when using topicNames.
+ String topic1 = "topic1";
+ String topic2 = "topic2";
+ List<String> topics = Arrays.asList(topic1, topic2);
+ DeleteTopicsRequest requestWithNames = new DeleteTopicsRequest.Builder(new DeleteTopicsRequestData()
+ .setTopicNames(topics)).build(version);
+ DeleteTopicsRequest requestWithNamesSerialized = DeleteTopicsRequest.parse(requestWithNames.serialize(), version);
+
+ assertEquals(topics, requestWithNames.topicNames());
+ assertEquals(topics, requestWithNamesSerialized.topicNames());
+
+ if (version < 6) {
+ assertEquals(topics, requestWithNames.data().topicNames());
+ assertEquals(topics, requestWithNamesSerialized.data().topicNames());
+ } else {
+ // topics in TopicNames are moved to new topics field
+ Iterator<String> topicsIterator = topics.iterator();
+ requestWithNames.data().topics().forEach(topic -> assertEquals(topicsIterator.next(), topic.name()));
+ Iterator<String> topicsIterator2 = topics.iterator();
+ requestWithNames.data().topics().forEach(topic -> assertEquals(topicsIterator2.next(), topic.name()));
+ }
+
+ // Ensure we only use new topics field on versions 6+.
+ try {
+ DeleteTopicsRequest requestWithNames2 = new DeleteTopicsRequest.Builder(new DeleteTopicsRequestData()
+ .setTopics(Arrays.asList(new DeleteTopicsRequestData.DeleteTopicState().setName(topic1),
+ new DeleteTopicsRequestData.DeleteTopicState().setName(topic2)))).build(version);
+ DeleteTopicsRequest requestWithNamesSerialized2 = DeleteTopicsRequest.parse(requestWithNames2.serialize(), version);
+
+ assertTrue(version >= 6);
+ assertEquals(topics, requestWithNames2.topicNames());
+ assertEquals(topics, requestWithNamesSerialized2.topicNames());
+
+ } catch (UnsupportedVersionException e) {
Review comment:
It would be better to use `assertThrows` instead of doing this because it is not clear where we expect this exception to be thrown. So, I would so something like the following:
```
if (version >= 6) {
// Do the regular part
} else {
// assertThrows with the expected code that must throw.
}
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jolshan commented on a change in pull request #10351: MINOR: use new method to get number of topics in DeleteTopicsRequest
Posted by GitBox <gi...@apache.org>.
jolshan commented on a change in pull request #10351:
URL: https://github.com/apache/kafka/pull/10351#discussion_r597088531
##########
File path: clients/src/main/java/org/apache/kafka/common/requests/DeleteTopicsRequest.java
##########
@@ -101,6 +101,12 @@ public AbstractResponse getErrorResponse(int throttleTimeMs, Throwable e) {
return data.topics().stream().map(topic -> topic.name()).collect(Collectors.toList());
return data.topicNames();
}
+
+ public int numberOfTopics() {
Review comment:
Can do!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on a change in pull request #10351: MINOR: use new method to get number of topics in DeleteTopicsRequest
Posted by GitBox <gi...@apache.org>.
dajac commented on a change in pull request #10351:
URL: https://github.com/apache/kafka/pull/10351#discussion_r597691255
##########
File path: clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java
##########
@@ -930,6 +930,28 @@ public void testCreateTopicRequestV3FailsIfNoPartitionsOrReplicas() {
assertTrue(exception.getMessage().contains("[foo, bar]"));
}
+ @Test
+ public void testDeleteTopicsRequestNumTopics() {
+ for (short version : DELETE_TOPICS.allVersions()) {
+ DeleteTopicsRequest request = createDeleteTopicsRequest(version);
+ DeleteTopicsRequest serializedRequest = DeleteTopicsRequest.parse(request.serialize(), version);
+ // createDeleteTopicsRequest sets 2 topics
+ assertEquals(2, request.numberOfTopics());
+ assertEquals(2, serializedRequest.numberOfTopics());
Review comment:
I suppose that this will fail if version >= 6, isn't it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on a change in pull request #10351: MINOR: use new method to get number of topics in DeleteTopicsRequest
Posted by GitBox <gi...@apache.org>.
dajac commented on a change in pull request #10351:
URL: https://github.com/apache/kafka/pull/10351#discussion_r597943854
##########
File path: clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java
##########
@@ -930,6 +930,28 @@ public void testCreateTopicRequestV3FailsIfNoPartitionsOrReplicas() {
assertTrue(exception.getMessage().contains("[foo, bar]"));
}
+ @Test
+ public void testDeleteTopicsRequestNumTopics() {
+ for (short version : DELETE_TOPICS.allVersions()) {
+ DeleteTopicsRequest request = createDeleteTopicsRequest(version);
+ DeleteTopicsRequest serializedRequest = DeleteTopicsRequest.parse(request.serialize(), version);
+ // createDeleteTopicsRequest sets 2 topics
+ assertEquals(2, request.numberOfTopics());
+ assertEquals(2, serializedRequest.numberOfTopics());
Review comment:
Yeah, we can add a DeleteTopicsRequestTest in the clients package for the unit tests.
As you said, DeleteTopicsRequestTest in core package is for the integration tests of the DeleteTopics API.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jolshan commented on a change in pull request #10351: MINOR: use new method to get number of topics in DeleteTopicsRequest
Posted by GitBox <gi...@apache.org>.
jolshan commented on a change in pull request #10351:
URL: https://github.com/apache/kafka/pull/10351#discussion_r597863428
##########
File path: clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java
##########
@@ -930,6 +930,28 @@ public void testCreateTopicRequestV3FailsIfNoPartitionsOrReplicas() {
assertTrue(exception.getMessage().contains("[foo, bar]"));
}
+ @Test
+ public void testDeleteTopicsRequestNumTopics() {
+ for (short version : DELETE_TOPICS.allVersions()) {
+ DeleteTopicsRequest request = createDeleteTopicsRequest(version);
+ DeleteTopicsRequest serializedRequest = DeleteTopicsRequest.parse(request.serialize(), version);
+ // createDeleteTopicsRequest sets 2 topics
+ assertEquals(2, request.numberOfTopics());
+ assertEquals(2, serializedRequest.numberOfTopics());
Review comment:
I think the reason I didn't put it in `DeleteTopicsRequestTest` is also because that is an integration test. Would it make sense to rename that and create a unit test called `DeleteTopicsRequestTest`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on a change in pull request #10351: MINOR: use new method to get number of topics in DeleteTopicsRequest
Posted by GitBox <gi...@apache.org>.
dajac commented on a change in pull request #10351:
URL: https://github.com/apache/kafka/pull/10351#discussion_r597859840
##########
File path: clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java
##########
@@ -930,6 +930,28 @@ public void testCreateTopicRequestV3FailsIfNoPartitionsOrReplicas() {
assertTrue(exception.getMessage().contains("[foo, bar]"));
}
+ @Test
+ public void testDeleteTopicsRequestNumTopics() {
+ for (short version : DELETE_TOPICS.allVersions()) {
+ DeleteTopicsRequest request = createDeleteTopicsRequest(version);
+ DeleteTopicsRequest serializedRequest = DeleteTopicsRequest.parse(request.serialize(), version);
+ // createDeleteTopicsRequest sets 2 topics
+ assertEquals(2, request.numberOfTopics());
+ assertEquals(2, serializedRequest.numberOfTopics());
Review comment:
Ah, right. I completely missed that part in the builder. We usually put unit tests for requests in their own classes (e.g. DeleteTopicsRequestTest) except the ones really verifying the serialisation which are here. I wonder if this one wouldn't be better in `DeleteTopicsRequestTest`. What do you think?
Slightly related, do we have a unit test verifying the normalization logic in the build?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on pull request #10351: MINOR: use new method to get number of topics in DeleteTopicsRequest
Posted by GitBox <gi...@apache.org>.
dajac commented on pull request #10351:
URL: https://github.com/apache/kafka/pull/10351#issuecomment-802846435
@jolshan You need to rebase to include a065fa5d1ac0e1775e472dcc6e9a9b138cb6639e. The build won't work otherwise.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jolshan commented on a change in pull request #10351: MINOR: use new method to get number of topics in DeleteTopicsRequest
Posted by GitBox <gi...@apache.org>.
jolshan commented on a change in pull request #10351:
URL: https://github.com/apache/kafka/pull/10351#discussion_r597863428
##########
File path: clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java
##########
@@ -930,6 +930,28 @@ public void testCreateTopicRequestV3FailsIfNoPartitionsOrReplicas() {
assertTrue(exception.getMessage().contains("[foo, bar]"));
}
+ @Test
+ public void testDeleteTopicsRequestNumTopics() {
+ for (short version : DELETE_TOPICS.allVersions()) {
+ DeleteTopicsRequest request = createDeleteTopicsRequest(version);
+ DeleteTopicsRequest serializedRequest = DeleteTopicsRequest.parse(request.serialize(), version);
+ // createDeleteTopicsRequest sets 2 topics
+ assertEquals(2, request.numberOfTopics());
+ assertEquals(2, serializedRequest.numberOfTopics());
Review comment:
I think the reason I didn't put it in DeleteTopicsRequestTest is also because that is an integration test. Would it make sense to rename that and create a unit test called DeleteTopicsRequestTest?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on a change in pull request #10351: MINOR: use new method to get number of topics in DeleteTopicsRequest
Posted by GitBox <gi...@apache.org>.
dajac commented on a change in pull request #10351:
URL: https://github.com/apache/kafka/pull/10351#discussion_r597073992
##########
File path: clients/src/main/java/org/apache/kafka/common/requests/DeleteTopicsRequest.java
##########
@@ -101,6 +101,12 @@ public AbstractResponse getErrorResponse(int throttleTimeMs, Throwable e) {
return data.topics().stream().map(topic -> topic.name()).collect(Collectors.toList());
return data.topicNames();
}
+
+ public int numberOfTopics() {
Review comment:
Should we add a small unit test or extend an existing unit test to verify this across all versions?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jolshan commented on pull request #10351: MINOR: use new method to get number of topics in DeleteTopicsRequest
Posted by GitBox <gi...@apache.org>.
jolshan commented on pull request #10351:
URL: https://github.com/apache/kafka/pull/10351#issuecomment-802271057
Build was broken due to an unrelated change, but should be fixed now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac merged pull request #10351: MINOR: use new method to get number of topics in DeleteTopicsRequest
Posted by GitBox <gi...@apache.org>.
dajac merged pull request #10351:
URL: https://github.com/apache/kafka/pull/10351
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jolshan commented on a change in pull request #10351: MINOR: use new method to get number of topics in DeleteTopicsRequest
Posted by GitBox <gi...@apache.org>.
jolshan commented on a change in pull request #10351:
URL: https://github.com/apache/kafka/pull/10351#discussion_r597861972
##########
File path: clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java
##########
@@ -930,6 +930,28 @@ public void testCreateTopicRequestV3FailsIfNoPartitionsOrReplicas() {
assertTrue(exception.getMessage().contains("[foo, bar]"));
}
+ @Test
+ public void testDeleteTopicsRequestNumTopics() {
+ for (short version : DELETE_TOPICS.allVersions()) {
+ DeleteTopicsRequest request = createDeleteTopicsRequest(version);
+ DeleteTopicsRequest serializedRequest = DeleteTopicsRequest.parse(request.serialize(), version);
+ // createDeleteTopicsRequest sets 2 topics
+ assertEquals(2, request.numberOfTopics());
+ assertEquals(2, serializedRequest.numberOfTopics());
Review comment:
I put it here because of the serialization code. (Which makes the topicNames field empty in versions 6+) But I can move it there too.
I don't think there is such a test for normalization logic, so I can add that too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org