Re: [PR] KAFKA-16484: Support to define per broker/controller property by ClusterConfigProperty [kafka]
chia7712 merged PR #15715: URL: https://github.com/apache/kafka/pull/15715 -- 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
Re: [PR] KAFKA-16484: Support to define per broker/controller property by ClusterConfigProperty [kafka]
chia7712 commented on PR #15715: URL: https://github.com/apache/kafka/pull/15715#issuecomment-2099699004 build get failed ... retrigger again -- 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
Re: [PR] KAFKA-16484: Support to define per broker/controller property by ClusterConfigProperty [kafka]
chia7712 commented on code in PR #15715: URL: https://github.com/apache/kafka/pull/15715#discussion_r1591692467 ## core/src/test/java/kafka/test/junit/ClusterTestExtensions.java: ## @@ -158,12 +162,24 @@ private void processClusterTest(ExtensionContext context, ClusterTest annot, Clu .setName(annot.name().trim().isEmpty() ? null : annot.name()) .setListenerName(annot.listener().trim().isEmpty() ? null : annot.listener()) .setServerProperties(serverProperties) +.setPerServerProperties(perServerProperties) .setSecurityProtocol(annot.securityProtocol()) .setMetadataVersion(annot.metadataVersion()) .build(); type.invocationContexts(context.getRequiredTestMethod().getName(), config, testInvocations); } +private void processClusterConfigProperty(ClusterConfigProperty property, Review Comment: Please remove this unused function. -- 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
Re: [PR] KAFKA-16484: Support to define per broker/controller property by ClusterConfigProperty [kafka]
chia7712 commented on code in PR #15715:
URL: https://github.com/apache/kafka/pull/15715#discussion_r1590605162
##
core/src/test/java/kafka/test/junit/ClusterTestExtensions.java:
##
@@ -143,11 +143,12 @@ private void processClusterTest(ExtensionContext context,
ClusterTest annot, Clu
Type type = annot.clusterType() == Type.DEFAULT ?
defaults.clusterType() : annot.clusterType();
Map serverProperties = new HashMap<>();
+Map> perServerProperties = new
HashMap<>();
Review Comment:
Maybe we can rewrite them by lambda.
```java
Map serverProperties =
Stream.concat(Arrays.stream(defaults.serverProperties()),
Arrays.stream(annot.serverProperties()))
.filter(e -> e.id() == -1)
.collect(Collectors.toMap(ClusterConfigProperty::key,
ClusterConfigProperty::value, (a, b) -> b));
Map> perServerProperties =
Stream.concat(Arrays.stream(defaults.serverProperties()),
Arrays.stream(annot.serverProperties()))
.filter(e -> e.id() != -1)
.collect(Collectors.groupingBy(ClusterConfigProperty::id,
Collectors.mapping(Function.identity(),
Collectors.toMap(ClusterConfigProperty::key,
ClusterConfigProperty::value, (a, b) -> b;
```
##
core/src/test/java/kafka/testkit/TestKitNodes.java:
##
@@ -105,17 +109,27 @@ public TestKitNodes build() {
List controllerNodeIds =
IntStream.range(startControllerId(), startControllerId() + numControllerNodes)
.boxed()
.collect(Collectors.toList());
-List brokerNodeIds = IntStream.range(startBrokerId(),
startBrokerId() + numBrokerNodes)
+List brokerNodeIds = IntStream.range(BROKER_ID_OFFSET,
BROKER_ID_OFFSET + numBrokerNodes)
.boxed()
.collect(Collectors.toList());
+Set unknownIds = perServerProperties.keySet().stream()
Review Comment:
We can convert the `Integer` to `String` here. The following error message
can use `String.join` to simplify code.
##
core/src/test/java/kafka/test/ClusterTestExtensionsTest.java:
##
@@ -80,30 +84,55 @@ public void testClusterTemplate() {
@ClusterTests({
@ClusterTest(name = "cluster-tests-1", clusterType = Type.ZK,
serverProperties = {
@ClusterConfigProperty(key = "foo", value = "bar"),
-@ClusterConfigProperty(key = "spam", value = "eggs")
+@ClusterConfigProperty(key = "spam", value = "eggs"),
+@ClusterConfigProperty(id = 86400, key = "baz", value = "qux"), //
this one will be ignored as there is no broker id is 86400
}),
@ClusterTest(name = "cluster-tests-2", clusterType = Type.KRAFT,
serverProperties = {
@ClusterConfigProperty(key = "foo", value = "baz"),
@ClusterConfigProperty(key = "spam", value = "eggz"),
-@ClusterConfigProperty(key = "default.key", value =
"overwrite.value")
+@ClusterConfigProperty(key = "default.key", value =
"overwrite.value"),
+@ClusterConfigProperty(id = 0, key = "queued.max.requests", value
= "200"),
+@ClusterConfigProperty(id = 3000, key = "queued.max.requests",
value = "300")
}),
@ClusterTest(name = "cluster-tests-3", clusterType = Type.CO_KRAFT,
serverProperties = {
@ClusterConfigProperty(key = "foo", value = "baz"),
@ClusterConfigProperty(key = "spam", value = "eggz"),
-@ClusterConfigProperty(key = "default.key", value =
"overwrite.value")
+@ClusterConfigProperty(key = "default.key", value =
"overwrite.value"),
+@ClusterConfigProperty(id = 0, key = "queued.max.requests", value
= "200")
})
})
-public void testClusterTests() {
-if
(clusterInstance.clusterType().equals(ClusterInstance.ClusterType.ZK)) {
+public void testClusterTests() throws ExecutionException,
InterruptedException {
+if (!clusterInstance.isKRaftTest()) {
Assertions.assertEquals("bar",
clusterInstance.config().serverProperties().get("foo"));
Assertions.assertEquals("eggs",
clusterInstance.config().serverProperties().get("spam"));
Assertions.assertEquals("default.value",
clusterInstance.config().serverProperties().get("default.key"));
-} else if
(clusterInstance.clusterType().equals(ClusterInstance.ClusterType.RAFT)) {
+
+try (Admin admin = clusterInstance.createAdminClient()) {
+ConfigResource configResource = new
ConfigResource(ConfigResource.Type.BROKER, "0");
+Map configs =
admin.describeConfigs(Collections.singletonList(configResource)).all().get();
+Assertions.assertEquals(1, configs.size());
+Assertions.assertEquals("100",
configs.get(configResource).get("queued.max.requests").value());
Review
Re: [PR] KAFKA-16484: Support to define per broker/controller property by ClusterConfigProperty [kafka]
chia7712 commented on code in PR #15715:
URL: https://github.com/apache/kafka/pull/15715#discussion_r1589861459
##
core/src/test/java/kafka/test/annotation/ClusterConfigProperty.java:
##
@@ -27,6 +27,23 @@
@Target({ElementType.ANNOTATION_TYPE})
@Retention(RetentionPolicy.RUNTIME)
public @interface ClusterConfigProperty {
+/**
+ * The config applies to the controller/broker with specified id. Default
is -1, indicating the property applied to
+ * all controller/broker servers. Note that the "controller" here refers
to the KRaft quorum controller.
+ * The id can vary depending on the different {@link
kafka.test.annotation.Type}.
+ *
+ * Under {@link kafka.test.annotation.Type#ZK}, the broker id starts
from 0 and increases by 1
+ * with each additional broker, and there is no controller server under
this mode.
+ * Under {@link kafka.test.annotation.Type#KRAFT}, the broker id
starts from 0, the controller id
+ * starts from 3000 and increases by 1 with each addition
broker/controller.
+ * Under {@link kafka.test.annotation.Type#CO_KRAFT}, the broker id
and controller id both start from 0
+ * and increases by 1 with each additional broker/controller.
+ *
+ *
+ * If the id doesn't correspond to any broker/controller server, the
config will be ignored.
Review Comment:
Maybe we should throw exception to do fast fail. WDTY?
##
core/src/test/java/kafka/testkit/TestKitNodes.java:
##
@@ -148,7 +151,7 @@ private int startControllerId() {
if (combined) {
return startBrokerId();
}
-return startBrokerId() + 3000;
+return startBrokerId() + CONTROLLER_ID_OFFSET;
Review Comment:
Maybe we should replace `startBrokerId` by a constant variable too.
##
core/src/test/java/kafka/test/annotation/ClusterConfigProperty.java:
##
@@ -27,6 +27,23 @@
@Target({ElementType.ANNOTATION_TYPE})
@Retention(RetentionPolicy.RUNTIME)
public @interface ClusterConfigProperty {
+/**
+ * The config applies to the controller/broker with specified id. Default
is -1, indicating the property applied to
+ * all controller/broker servers. Note that the "controller" here refers
to the KRaft quorum controller.
+ * The id can vary depending on the different {@link
kafka.test.annotation.Type}.
+ *
+ * Under {@link kafka.test.annotation.Type#ZK}, the broker id starts
from 0 and increases by 1
+ * with each additional broker, and there is no controller server under
this mode.
+ * Under {@link kafka.test.annotation.Type#KRAFT}, the broker id
starts from 0, the controller id
+ * starts from 3000 and increases by 1 with each addition
broker/controller.
Review Comment:
`3000` can be replaced by the `{@link }`
##
core/src/test/java/kafka/test/ClusterTestExtensionsTest.java:
##
@@ -80,28 +84,55 @@ public void testClusterTemplate() {
@ClusterTests({
@ClusterTest(name = "cluster-tests-1", clusterType = Type.ZK,
serverProperties = {
@ClusterConfigProperty(key = "foo", value = "bar"),
-@ClusterConfigProperty(key = "spam", value = "eggs")
+@ClusterConfigProperty(key = "spam", value = "eggs"),
+@ClusterConfigProperty(id = 86400, key = "baz", value = "qux"), //
this one will be ignored as there is no broker id is 86400
}),
@ClusterTest(name = "cluster-tests-2", clusterType = Type.KRAFT,
serverProperties = {
@ClusterConfigProperty(key = "foo", value = "baz"),
@ClusterConfigProperty(key = "spam", value = "eggz"),
-@ClusterConfigProperty(key = "default.key", value =
"overwrite.value")
+@ClusterConfigProperty(key = "default.key", value =
"overwrite.value"),
+@ClusterConfigProperty(id = 0, key = "queued.max.requests", value
= "200"),
+@ClusterConfigProperty(id = 3000, key = "queued.max.requests",
value = "300")
}),
@ClusterTest(name = "cluster-tests-3", clusterType = Type.CO_KRAFT,
serverProperties = {
@ClusterConfigProperty(key = "foo", value = "baz"),
@ClusterConfigProperty(key = "spam", value = "eggz"),
-@ClusterConfigProperty(key = "default.key", value =
"overwrite.value")
+@ClusterConfigProperty(key = "default.key", value =
"overwrite.value"),
+@ClusterConfigProperty(id = 0, key = "queued.max.requests", value
= "200")
})
})
-public void testClusterTests() {
+public void testClusterTests() throws ExecutionException,
InterruptedException {
if
(clusterInstance.clusterType().equals(ClusterInstance.ClusterType.ZK)) {
Assertions.assertEquals("bar",
clusterInstance.config().serverProperties().get("foo"));
Assertions.assertEquals("eggs",
clusterInstance.config().serverProperties().get("spam"));
Re: [PR] KAFKA-16484: Support to define per broker/controller property by ClusterConfigProperty [kafka]
chia7712 commented on PR #15715: URL: https://github.com/apache/kafka/pull/15715#issuecomment-2092381354 @brandboat Please fix the conflicts -- 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
Re: [PR] KAFKA-16484: Support to define per broker/controller property by ClusterConfigProperty [kafka]
brandboat commented on code in PR #15715:
URL: https://github.com/apache/kafka/pull/15715#discussion_r1584685682
##
core/src/test/java/kafka/test/ClusterConfig.java:
##
@@ -172,14 +179,16 @@ public boolean equals(Object object) {
&& Objects.equals(adminClientProperties,
clusterConfig.adminClientProperties)
&& Objects.equals(saslServerProperties,
clusterConfig.saslServerProperties)
&& Objects.equals(saslClientProperties,
clusterConfig.saslClientProperties)
-&& Objects.equals(perBrokerOverrideProperties,
clusterConfig.perBrokerOverrideProperties);
+&& Objects.equals(perBrokerOverrideProperties,
clusterConfig.perBrokerOverrideProperties)
+&& Objects.equals(perControllerOverrideProperties,
clusterConfig.perControllerOverrideProperties);
}
@Override
public int hashCode() {
return Objects.hash(type, brokers, controllers, name, autoStart,
securityProtocol, listenerName,
trustStoreFile, metadataVersion, serverProperties,
producerProperties, consumerProperties,
-adminClientProperties, saslServerProperties,
saslClientProperties, perBrokerOverrideProperties);
+adminClientProperties, saslServerProperties,
saslClientProperties, perBrokerOverrideProperties,
+perControllerOverrideProperties);
Review Comment:
> Do we need equals and hashCode, or we can just remove it?
Just remove it , as you already done in the pr.
--
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
Re: [PR] KAFKA-16484: Support to define per broker/controller property by ClusterConfigProperty [kafka]
FrankYang0529 commented on code in PR #15715:
URL: https://github.com/apache/kafka/pull/15715#discussion_r1584677103
##
core/src/test/java/kafka/test/ClusterConfig.java:
##
@@ -172,14 +179,16 @@ public boolean equals(Object object) {
&& Objects.equals(adminClientProperties,
clusterConfig.adminClientProperties)
&& Objects.equals(saslServerProperties,
clusterConfig.saslServerProperties)
&& Objects.equals(saslClientProperties,
clusterConfig.saslClientProperties)
-&& Objects.equals(perBrokerOverrideProperties,
clusterConfig.perBrokerOverrideProperties);
+&& Objects.equals(perBrokerOverrideProperties,
clusterConfig.perBrokerOverrideProperties)
+&& Objects.equals(perControllerOverrideProperties,
clusterConfig.perControllerOverrideProperties);
}
@Override
public int hashCode() {
return Objects.hash(type, brokers, controllers, name, autoStart,
securityProtocol, listenerName,
trustStoreFile, metadataVersion, serverProperties,
producerProperties, consumerProperties,
-adminClientProperties, saslServerProperties,
saslClientProperties, perBrokerOverrideProperties);
+adminClientProperties, saslServerProperties,
saslClientProperties, perBrokerOverrideProperties,
+perControllerOverrideProperties);
Review Comment:
Do we need `equals` and `hashCode`, or we can just remove it?
Ref: https://github.com/apache/kafka/pull/15745#discussion_r1582249405
--
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
Re: [PR] KAFKA-16484: Support to define per broker/controller property by ClusterConfigProperty [kafka]
chia7712 commented on code in PR #15715:
URL: https://github.com/apache/kafka/pull/15715#discussion_r1584322426
##
core/src/test/java/kafka/test/junit/ClusterTestExtensions.java:
##
@@ -196,16 +197,35 @@ private void processClusterTest(ExtensionContext context,
ClusterTest annot, Clu
}
Map serverProperties = new HashMap<>();
+Map> perBrokerProperties = new
HashMap<>();
+Map> perControllerProperties = new
HashMap<>();
for (ClusterConfigProperty property : defaults.serverProperties()) {
-serverProperties.put(property.key(), property.value());
+processClusterConfigProperty(property, serverProperties,
perBrokerProperties, perControllerProperties);
}
for (ClusterConfigProperty property : annot.serverProperties()) {
-serverProperties.put(property.key(), property.value());
+processClusterConfigProperty(property, serverProperties,
perBrokerProperties, perControllerProperties);
}
-configBuilder.setServerProperties(serverProperties);
+configBuilder.setServerProperties(serverProperties)
+.setPerBrokerProperties(perBrokerProperties)
+.setPerControllerProperties(perControllerProperties);
type.invocationContexts(context.getRequiredTestMethod().getName(),
configBuilder.build(), testInvocations);
}
+private void processClusterConfigProperty(ClusterConfigProperty property,
+ Map
serverProperties,
+ Map> perBrokerProperties,
+ Map> perControllerProperties) {
+if (property.id() == -1) {
+serverProperties.put(property.key(), property.value());
+} else if (property.id() >= TestKitNodes.CONTROLLER_ID_OFFSET) {
Review Comment:
It seems we don't need to have two per"xxx"properties. In kraft mode, broker
and quorum controller have different id. In kraft-combined mode, the combined
broker/controller share the same config.
Hence, we can keep one `perServerProerties`.
--
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
Re: [PR] KAFKA-16484: Support to define per broker/controller property by ClusterConfigProperty [kafka]
chia7712 commented on code in PR #15715: URL: https://github.com/apache/kafka/pull/15715#discussion_r1566895408 ## core/src/test/java/kafka/test/junit/ClusterTestExtensions.java: ## @@ -190,10 +192,10 @@ private void processClusterTest(ExtensionContext context, ClusterTest annot, Clu ClusterConfig config = builder.build(); Review Comment: > OK, I'll also modify the ControllerNode part, thanks ! Do you want to do refactor in this PR? or you can complete the refactor in https://issues.apache.org/jira/browse/KAFKA-16560 first. If you prefer to address them at once, I'm ok to close https://issues.apache.org/jira/browse/KAFKA-16560 -- 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
Re: [PR] KAFKA-16484: Support to define per broker/controller property by ClusterConfigProperty [kafka]
brandboat commented on code in PR #15715: URL: https://github.com/apache/kafka/pull/15715#discussion_r1566900542 ## core/src/test/java/kafka/test/junit/ClusterTestExtensions.java: ## @@ -190,10 +192,10 @@ private void processClusterTest(ExtensionContext context, ClusterTest annot, Clu ClusterConfig config = builder.build(); Review Comment: I'll address other comments you mentioned excepts the refactoring part tonight. Thanks for your patience. -- 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
Re: [PR] KAFKA-16484: Support to define per broker/controller property by ClusterConfigProperty [kafka]
brandboat commented on code in PR #15715: URL: https://github.com/apache/kafka/pull/15715#discussion_r1566898370 ## core/src/test/java/kafka/test/junit/ClusterTestExtensions.java: ## @@ -190,10 +192,10 @@ private void processClusterTest(ExtensionContext context, ClusterTest annot, Clu ClusterConfig config = builder.build(); Review Comment: Prefer doing the refactoring part in https://issues.apache.org/jira/browse/KAFKA-16560 since that requires bunch of works to 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. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16484: Support to define per broker/controller property by ClusterConfigProperty [kafka]
brandboat commented on code in PR #15715: URL: https://github.com/apache/kafka/pull/15715#discussion_r1566887858 ## core/src/test/java/kafka/test/junit/ClusterTestExtensions.java: ## @@ -190,10 +192,10 @@ private void processClusterTest(ExtensionContext context, ClusterTest annot, Clu ClusterConfig config = builder.build(); Review Comment: OK, I'll also modify the `ControllerNode` part, thanks ! -- 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
Re: [PR] KAFKA-16484: Support to define per broker/controller property by ClusterConfigProperty [kafka]
brandboat commented on code in PR #15715: URL: https://github.com/apache/kafka/pull/15715#discussion_r1566877392 ## core/src/test/java/kafka/test/junit/ClusterTestExtensions.java: ## @@ -190,10 +192,10 @@ private void processClusterTest(ExtensionContext context, ClusterTest annot, Clu ClusterConfig config = builder.build(); Review Comment: Thanks [Chia-Ping Tsai] ! In this pr, currently I'm working on making ClusterConfig immutable. And other places as you mentioned like `BrokerNode`, `ControllerNode` are not immutable. We can continue the work in the JIRA you just filed. -- 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
Re: [PR] KAFKA-16484: Support to define per broker/controller property by ClusterConfigProperty [kafka]
chia7712 commented on code in PR #15715: URL: https://github.com/apache/kafka/pull/15715#discussion_r1566884610 ## core/src/test/java/kafka/test/junit/ClusterTestExtensions.java: ## @@ -190,10 +192,10 @@ private void processClusterTest(ExtensionContext context, ClusterTest annot, Clu ClusterConfig config = builder.build(); Review Comment: It seems this PR includes the changes to modify `ControllerNode` directly, and that is way I feel the refactor should be completed first. -- 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
Re: [PR] KAFKA-16484: Support to define per broker/controller property by ClusterConfigProperty [kafka]
brandboat commented on code in PR #15715: URL: https://github.com/apache/kafka/pull/15715#discussion_r1566877392 ## core/src/test/java/kafka/test/junit/ClusterTestExtensions.java: ## @@ -190,10 +192,10 @@ private void processClusterTest(ExtensionContext context, ClusterTest annot, Clu ClusterConfig config = builder.build(); Review Comment: Thanks @chia7712! In this pr, currently I'm working on making ClusterConfig immutable. And other places as you mentioned like `BrokerNode`, `ControllerNode` are not immutable. We can continue the work in the JIRA you just filed. -- 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
Re: [PR] KAFKA-16484: Support to define per broker/controller property by ClusterConfigProperty [kafka]
chia7712 commented on code in PR #15715: URL: https://github.com/apache/kafka/pull/15715#discussion_r1566868413 ## core/src/test/java/kafka/test/junit/ClusterTestExtensions.java: ## @@ -190,10 +192,10 @@ private void processClusterTest(ExtensionContext context, ClusterTest annot, Clu ClusterConfig config = builder.build(); Review Comment: I have filed a jira https://issues.apache.org/jira/browse/KAFKA-16560 -- 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
Re: [PR] KAFKA-16484: Support to define per broker/controller property by ClusterConfigProperty [kafka]
chia7712 commented on code in PR #15715: URL: https://github.com/apache/kafka/pull/15715#discussion_r1564660916 ## core/src/test/java/kafka/test/junit/ClusterTestExtensions.java: ## @@ -190,10 +192,10 @@ private void processClusterTest(ExtensionContext context, ClusterTest annot, Clu ClusterConfig config = builder.build(); Review Comment: > Sure, but that could require some efforts here, there are plenty of places directly invoke ClusterConfig#serverProperties and add server properties before cluster start. e.g. KafkaServerKRaftRegistrationTest. yep, but it is worth the effort. We adopt the builder pattern already, so the built object should be immutable. If the refactor could includes huge changes, we can have a separate PR for that. Or we can refactor them one by one. 1. `ClusterConfig` 2. `BrokerNode` 3. `ControllerNode` -- 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
Re: [PR] KAFKA-16484: Support to define per broker/controller property by ClusterConfigProperty [kafka]
brandboat commented on code in PR #15715: URL: https://github.com/apache/kafka/pull/15715#discussion_r1564660683 ## core/src/test/java/kafka/test/junit/ClusterTestExtensions.java: ## @@ -190,10 +192,10 @@ private void processClusterTest(ExtensionContext context, ClusterTest annot, Clu ClusterConfig config = builder.build(); Review Comment: There are places like https://github.com/apache/kafka/blob/0b4e9afee2ace7edf6ff8690e070100b98627836/core/src/test/scala/integration/kafka/server/KafkaServerKRaftRegistrationTest.scala#L74 need to add extra properties and then restart cluster. If we make ClusterConfig immutable, this may requires more effort to think about how do we handle this scenario. What I want to say is the work could be huge, and overwhelm what we want to address in this JIRA. i.e. define per broker/controller property -- 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
Re: [PR] KAFKA-16484: Support to define per broker/controller property by ClusterConfigProperty [kafka]
brandboat commented on code in PR #15715: URL: https://github.com/apache/kafka/pull/15715#discussion_r1564654806 ## core/src/test/java/kafka/test/junit/ClusterTestExtensions.java: ## @@ -190,10 +192,10 @@ private void processClusterTest(ExtensionContext context, ClusterTest annot, Clu ClusterConfig config = builder.build(); Review Comment: Sure, but that could require some efforts here, there are plenty of places directly invoke `ClusterConfig#serverProperties` and add server properties before cluster start. e.g. `KafkaServerKRaftRegistrationTest`. -- 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
Re: [PR] KAFKA-16484: Support to define per broker/controller property by ClusterConfigProperty [kafka]
chia7712 commented on code in PR #15715:
URL: https://github.com/apache/kafka/pull/15715#discussion_r1564305286
##
core/src/test/java/kafka/test/junit/RaftClusterInvocationContext.java:
##
@@ -99,6 +99,10 @@ public List getAdditionalExtensions() {
clusterConfig.brokerServerProperties(brokerId).forEach(
(key, value) ->
brokerNode.propertyOverrides().put(key.toString(), value.toString()));
});
+nodes.controllerNodes().forEach((controllerId, controllerNode)
-> {
Review Comment:
`{` is redundant.
##
core/src/test/java/kafka/test/junit/ClusterTestExtensions.java:
##
@@ -203,6 +205,16 @@ private ClusterTestDefaults
getClusterTestDefaults(Class testClass) {
.orElseGet(() ->
EmptyClass.class.getDeclaredAnnotation(ClusterTestDefaults.class));
}
+private static void applyConfig(ClusterConfig config,
ClusterConfigProperty property) {
+if (property.id() == -1) {
+config.serverProperties().put(property.key(), property.value());
+} else if (property.id() >= CONTROLLER_ID_OFFSET) {
Review Comment:
We need to document it to make sure developers are aware of
broker/controller id when they try to define per broker/controller configs
##
core/src/test/java/kafka/test/junit/ClusterTestExtensions.java:
##
@@ -190,10 +192,10 @@ private void processClusterTest(ExtensionContext context,
ClusterTest annot, Clu
ClusterConfig config = builder.build();
Review Comment:
Could you please refactor the code to make `ClusterConfig` immutable? It
seems be a chaos that we pass mutable objects everywhere ...
--
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
[PR] KAFKA-16484: Support to define per broker/controller property by ClusterConfigProperty [kafka]
brandboat opened a new pull request, #15715: URL: https://github.com/apache/kafka/pull/15715 related to KAFKA-16484 Introduce a new field `id` in annotation `ClusterConfigProperty`. The main purpose of new field is to define specific broker/controller(kraft) property. And the default value is `-1` which means the ClusterConfigProperty will apply to all broker/controller. Note that under [Type.KRAFT](https://github.com/apache/kafka/blob/c034cf2953e691ce4ecd94bf00ac5810167354bc/core/src/test/java/kafka/test/annotation/Type.java#L31) mode, the controller id [starts from 3000](https://github.com/apache/kafka/blob/c034cf2953e691ce4ecd94bf00ac5810167354bc/core/src/test/java/kafka/testkit/TestKitNodes.java#L141-L146), and then increments by one each time. Other modes the broker/controller id starts from 0 and then increments by one. ### 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. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
