You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "FrankYang0529 (via GitHub)" <gi...@apache.org> on 2024/04/09 12:38:56 UTC

[PR] KAFKA-16487: Support to define server properties by ClusterTestDefaults [kafka]

FrankYang0529 opened a new pull request, #15687:
URL: https://github.com/apache/kafka/pull/15687

   Sometimes we want to define server properties for all test cases, and using `BeforeEach` to modify the `ClusterConfig` is the only way. The side effect is that the IDE does not like the style since IDE can't recognize custom ParameterResolver of `ClusterConfig`.
   
   The alternative is that we can take `ClusterInstance` from constructor first, and then we modify the inner `ClusterConfig` in `BeforeEach` phase. However, that may confuse users about the life cycle of "configs".
   
   In short, I prefer to define the server property by `ClusterTestDefaults`. It already includes some server-side default property, and we can enhance that to deal with more existent test case.
   
   ### 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-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] KAFKA-16487: Support to define server properties by ClusterTestDefaults [kafka]

Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 merged PR #15687:
URL: https://github.com/apache/kafka/pull/15687


-- 
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-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] KAFKA-16487: Support to define server properties by ClusterTestDefaults [kafka]

Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #15687:
URL: https://github.com/apache/kafka/pull/15687#discussion_r1557576225


##########
core/src/test/java/kafka/test/annotation/ClusterTestDefaults.java:
##########
@@ -39,4 +39,5 @@
     int brokers() default 1;
     int controllers() default 1;
     boolean autoStart() default true;
+    ClusterConfigProperty[] serverProperties() default {};

Review Comment:
   Could you add comments for this new fields?



-- 
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-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] KAFKA-16487: Support to define server properties by ClusterTestDefaults [kafka]

Posted by "FrankYang0529 (via GitHub)" <gi...@apache.org>.
FrankYang0529 commented on code in PR #15687:
URL: https://github.com/apache/kafka/pull/15687#discussion_r1557742410


##########
core/src/test/java/kafka/test/junit/ClusterTestExtensions.java:
##########
@@ -190,6 +190,9 @@ private void processClusterTest(ExtensionContext context, ClusterTest annot, Clu
         }
 
         Properties properties = new Properties();
+        for (ClusterConfigProperty property : defaults.serverProperties()) {

Review Comment:
   Updated it. Thank you.



-- 
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-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] KAFKA-16487: Support to define server properties by ClusterTestDefaults [kafka]

Posted by "FrankYang0529 (via GitHub)" <gi...@apache.org>.
FrankYang0529 commented on code in PR #15687:
URL: https://github.com/apache/kafka/pull/15687#discussion_r1557588875


##########
core/src/test/java/kafka/test/annotation/ClusterTestDefaults.java:
##########
@@ -39,4 +39,5 @@
     int brokers() default 1;
     int controllers() default 1;
     boolean autoStart() default true;
+    ClusterConfigProperty[] serverProperties() default {};

Review Comment:
   Yes, updated it. Thank you.



-- 
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-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] KAFKA-16487: Support to define server properties by ClusterTestDefaults [kafka]

Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #15687:
URL: https://github.com/apache/kafka/pull/15687#discussion_r1557600867


##########
core/src/test/java/kafka/test/junit/ClusterTestExtensions.java:
##########
@@ -190,6 +190,9 @@ private void processClusterTest(ExtensionContext context, ClusterTest annot, Clu
         }
 
         Properties properties = new Properties();
+        for (ClusterConfigProperty property : defaults.serverProperties()) {

Review Comment:
   this is unrelated to this PR, but could we update `config.serverProperties` instead of creating a temporary `properties`?



-- 
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-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] KAFKA-16487: Support to define server properties by ClusterTestDefaults [kafka]

Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #15687:
URL: https://github.com/apache/kafka/pull/15687#discussion_r1557608588


##########
core/src/test/java/kafka/test/ClusterTestExtensionsTest.java:
##########
@@ -89,20 +91,24 @@ public void testClusterTemplate() {
         }),
         @ClusterTest(name = "cluster-tests-2", clusterType = Type.KRAFT, serverProperties = {
             @ClusterConfigProperty(key = "foo", value = "baz"),
-            @ClusterConfigProperty(key = "spam", value = "eggz")
+            @ClusterConfigProperty(key = "spam", value = "eggz"),
+            @ClusterConfigProperty(key = "default.key", value = "overwrite.value")
         }),
         @ClusterTest(name = "cluster-tests-3", clusterType = Type.CO_KRAFT, serverProperties = {
             @ClusterConfigProperty(key = "foo", value = "baz"),
-            @ClusterConfigProperty(key = "spam", value = "eggz")
+            @ClusterConfigProperty(key = "spam", value = "eggz"),
+            @ClusterConfigProperty(key = "default.key", value = "overwrite.value")
         })
     })
     public void testClusterTests() {
         if (clusterInstance.clusterType().equals(ClusterInstance.ClusterType.ZK)) {
             Assertions.assertEquals(clusterInstance.config().serverProperties().getProperty("foo"), "bar");
             Assertions.assertEquals(clusterInstance.config().serverProperties().getProperty("spam"), "eggs");
+            Assertions.assertEquals(clusterInstance.config().serverProperties().getProperty("default.key"), "default.value");

Review Comment:
   the first argument is "expected", and so `default.value` should be the first input.



-- 
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-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] KAFKA-16487: Support to define server properties by ClusterTestDefaults [kafka]

Posted by "FrankYang0529 (via GitHub)" <gi...@apache.org>.
FrankYang0529 commented on code in PR #15687:
URL: https://github.com/apache/kafka/pull/15687#discussion_r1557742879


##########
core/src/test/java/kafka/test/ClusterTestExtensionsTest.java:
##########
@@ -89,20 +91,24 @@ public void testClusterTemplate() {
         }),
         @ClusterTest(name = "cluster-tests-2", clusterType = Type.KRAFT, serverProperties = {
             @ClusterConfigProperty(key = "foo", value = "baz"),
-            @ClusterConfigProperty(key = "spam", value = "eggz")
+            @ClusterConfigProperty(key = "spam", value = "eggz"),
+            @ClusterConfigProperty(key = "default.key", value = "overwrite.value")
         }),
         @ClusterTest(name = "cluster-tests-3", clusterType = Type.CO_KRAFT, serverProperties = {
             @ClusterConfigProperty(key = "foo", value = "baz"),
-            @ClusterConfigProperty(key = "spam", value = "eggz")
+            @ClusterConfigProperty(key = "spam", value = "eggz"),
+            @ClusterConfigProperty(key = "default.key", value = "overwrite.value")
         })
     })
     public void testClusterTests() {
         if (clusterInstance.clusterType().equals(ClusterInstance.ClusterType.ZK)) {
             Assertions.assertEquals(clusterInstance.config().serverProperties().getProperty("foo"), "bar");
             Assertions.assertEquals(clusterInstance.config().serverProperties().getProperty("spam"), "eggs");
+            Assertions.assertEquals(clusterInstance.config().serverProperties().getProperty("default.key"), "default.value");

Review Comment:
   Changed the input order. Thank you.



-- 
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-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org