hachikuji commented on code in PR #12106:
URL: https://github.com/apache/kafka/pull/12106#discussion_r863259182
##########
core/src/test/scala/unit/kafka/server/ControllerApisTest.scala:
##########
@@ -730,8 +729,45 @@ class ControllerApisTest {
request.topics().add(new
CreatePartitionsTopic().setName("bar").setAssignments(null).setCount(5))
request.topics().add(new
CreatePartitionsTopic().setName("baz").setAssignments(null).setCount(5))
assertEquals(Set(new CreatePartitionsTopicResult().setName("foo").
- setErrorCode(NONE.code()).
- setErrorMessage(null),
+ setErrorCode(NONE.code()).
+ setErrorMessage(null),
+ new CreatePartitionsTopicResult().setName("bar").
+ setErrorCode(INVALID_REQUEST.code()).
+ setErrorMessage("Duplicate topic name."),
+ new CreatePartitionsTopicResult().setName("baz").
+ setErrorCode(TOPIC_AUTHORIZATION_FAILED.code()).
+ setErrorMessage(null)),
+ controllerApis.createPartitions(ANONYMOUS_CONTEXT, request,
+ _ => Set("foo", "bar")).get().asScala.toSet)
+ }
+
+ @Test
+ def testValidateOnlyCreatePartitionsRequest(): Unit = {
Review Comment:
Is this test basically the same as `testCreatePartitionsRequest`? Maybe we
can get rid of one and turn the other into a `@ParameterizedTest` with
`validateOnly` as the parameter?
##########
metadata/src/main/java/org/apache/kafka/controller/Controller.java:
##########
@@ -328,11 +328,15 @@ CompletableFuture<UpdateFeaturesResponseData>
updateFeatures(
* Create partitions on certain topics.
*
* @param topics The list of topics to create partitions for.
+ * @param validateOnly If true, create partitions is just validated and
returns response
Review Comment:
nit: how about this?
> If true, the request is validated, but no partitions will be created.
##########
metadata/src/main/java/org/apache/kafka/controller/ReplicationControlManager.java:
##########
@@ -1364,7 +1364,8 @@ Boolean isBrokerUnfenced(int brokerId) {
setErrorCode(apiError.error().code()).
setErrorMessage(apiError.message()));
}
- return new ControllerResult<>(records, results, true);
+ log.debug("CreatePartitions result(s): {}", results);
Review Comment:
Hmm.. It is useful to know in the logs if `validateOnly` was set.
##########
core/src/test/scala/integration/kafka/api/PlaintextAdminIntegrationTest.scala:
##########
@@ -499,23 +504,28 @@ class PlaintextAdminIntegrationTest extends
BaseAdminIntegrationTest {
e = assertThrows(classOf[ExecutionException], () =>
alterResult.values.get(topic2).get,
() => s"$desc: Expect InvalidPartitionsException when requesting a
noop")
assertTrue(e.getCause.isInstanceOf[InvalidPartitionsException], desc)
- assertEquals("Topic already has 3 partitions.", e.getCause.getMessage,
desc)
+ exceptionMsgStr = if (TestInfoUtils.isKRaft(testInfo)) {
+ "Topic already has 3 partition(s)."
+ } else {
+ "Topic already has 3 partitions."
Review Comment:
I wonder if we can unify the error messages? The differences do not seem
interesting.
--
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.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
