Re: [PR] KAFKA-16482: Eliminate the IDE warnings of accepting ClusterConfig in BeforeEach [kafka]

2024-04-11 Thread via GitHub 


chia7712 merged PR #15676:
URL: https://github.com/apache/kafka/pull/15676


-- 
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-16482: Eliminate the IDE warnings of accepting ClusterConfig in BeforeEach [kafka]

2024-04-10 Thread via GitHub 


KevinZTW commented on code in PR #15676:
URL: https://github.com/apache/kafka/pull/15676#discussion_r1560373161


##
core/src/test/scala/integration/kafka/coordinator/transaction/ProducerIdsIntegrationTest.scala:
##
@@ -19,32 +19,31 @@ package kafka.coordinator.transaction
 
 import kafka.network.SocketServer
 import kafka.server.{IntegrationTestUtils, KafkaConfig}
-import kafka.test.annotation.{AutoStart, ClusterTest, ClusterTests, Type}
+import kafka.test.ClusterInstance
+import kafka.test.annotation.{AutoStart, ClusterConfigProperty, ClusterTest, 
ClusterTestDefaults, ClusterTests, Type}
 import kafka.test.junit.ClusterTestExtensions
-import kafka.test.{ClusterConfig, ClusterInstance}
 import org.apache.kafka.common.message.InitProducerIdRequestData
 import org.apache.kafka.common.network.ListenerName
 import org.apache.kafka.common.protocol.Errors
 import org.apache.kafka.common.record.RecordBatch
 import org.apache.kafka.common.requests.{InitProducerIdRequest, 
InitProducerIdResponse}
 import org.apache.kafka.server.common.MetadataVersion
 import org.junit.jupiter.api.Assertions.{assertEquals, assertTrue}
-import org.junit.jupiter.api.{BeforeEach, Disabled, Timeout}
 import org.junit.jupiter.api.extension.ExtendWith
+import org.junit.jupiter.api.{Disabled, Timeout}
 
 import java.util.stream.{Collectors, IntStream}
 import scala.concurrent.duration.DurationInt
 import scala.jdk.CollectionConverters._
 
+
+@ClusterTestDefaults(serverProperties = Array(
+  new ClusterConfigProperty(key = "transaction.state.log.num.partitions", 
value = "1"),
+  new ClusterConfigProperty(key = "transaction.state.log.replication.factor", 
value = "3")

Review Comment:
   Oh thanks for letting me know! I check the `KafkaConfig` and as you said, it 
would use the `TransactionLogConfig.DEFAULT_REPLICATION_FACTOR` when defining 
the config
   
   



-- 
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-16482: Eliminate the IDE warnings of accepting ClusterConfig in BeforeEach [kafka]

2024-04-10 Thread via GitHub 


chia7712 commented on code in PR #15676:
URL: https://github.com/apache/kafka/pull/15676#discussion_r1559385018


##
core/src/test/scala/integration/kafka/coordinator/transaction/ProducerIdsIntegrationTest.scala:
##
@@ -19,32 +19,31 @@ package kafka.coordinator.transaction
 
 import kafka.network.SocketServer
 import kafka.server.{IntegrationTestUtils, KafkaConfig}
-import kafka.test.annotation.{AutoStart, ClusterTest, ClusterTests, Type}
+import kafka.test.ClusterInstance
+import kafka.test.annotation.{AutoStart, ClusterConfigProperty, ClusterTest, 
ClusterTestDefaults, ClusterTests, Type}
 import kafka.test.junit.ClusterTestExtensions
-import kafka.test.{ClusterConfig, ClusterInstance}
 import org.apache.kafka.common.message.InitProducerIdRequestData
 import org.apache.kafka.common.network.ListenerName
 import org.apache.kafka.common.protocol.Errors
 import org.apache.kafka.common.record.RecordBatch
 import org.apache.kafka.common.requests.{InitProducerIdRequest, 
InitProducerIdResponse}
 import org.apache.kafka.server.common.MetadataVersion
 import org.junit.jupiter.api.Assertions.{assertEquals, assertTrue}
-import org.junit.jupiter.api.{BeforeEach, Disabled, Timeout}
 import org.junit.jupiter.api.extension.ExtendWith
+import org.junit.jupiter.api.{Disabled, Timeout}
 
 import java.util.stream.{Collectors, IntStream}
 import scala.concurrent.duration.DurationInt
 import scala.jdk.CollectionConverters._
 
+
+@ClusterTestDefaults(serverProperties = Array(
+  new ClusterConfigProperty(key = "transaction.state.log.num.partitions", 
value = "1"),
+  new ClusterConfigProperty(key = "transaction.state.log.replication.factor", 
value = "3")

Review Comment:
   this is equal to the default value 
(https://github.com/apache/kafka/blob/trunk/transaction-coordinator/src/main/java/org/apache/kafka/coordinator/transaction/TransactionLogConfig.java#L23)
 , so maybe we can remove 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.

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-16482: Eliminate the IDE warnings of accepting ClusterConfig in BeforeEach [kafka]

2024-04-09 Thread via GitHub 


KevinZTW commented on code in PR #15676:
URL: https://github.com/apache/kafka/pull/15676#discussion_r1558864630


##
core/src/test/scala/integration/kafka/coordinator/transaction/ProducerIdsIntegrationTest.scala:
##
@@ -19,30 +19,30 @@ package kafka.coordinator.transaction
 
 import kafka.network.SocketServer
 import kafka.server.{IntegrationTestUtils, KafkaConfig}
+import kafka.test.ClusterInstance
 import kafka.test.annotation.{AutoStart, ClusterTest, ClusterTests, Type}
 import kafka.test.junit.ClusterTestExtensions
-import kafka.test.{ClusterConfig, ClusterInstance}
 import org.apache.kafka.common.message.InitProducerIdRequestData
 import org.apache.kafka.common.network.ListenerName
 import org.apache.kafka.common.protocol.Errors
 import org.apache.kafka.common.record.RecordBatch
 import org.apache.kafka.common.requests.{InitProducerIdRequest, 
InitProducerIdResponse}
 import org.apache.kafka.server.common.MetadataVersion
 import org.junit.jupiter.api.Assertions.{assertEquals, assertTrue}
-import org.junit.jupiter.api.{BeforeEach, Disabled, Timeout}
 import org.junit.jupiter.api.extension.ExtendWith
+import org.junit.jupiter.api.{BeforeEach, Disabled, Timeout}
 
 import java.util.stream.{Collectors, IntStream}
 import scala.concurrent.duration.DurationInt
 import scala.jdk.CollectionConverters._
 
 @ExtendWith(value = Array(classOf[ClusterTestExtensions]))
-class ProducerIdsIntegrationTest {
+class ProducerIdsIntegrationTest(private val cluster: ClusterInstance) {

Review Comment:
   Oh no porblem, sorry I miss this one!



##
core/src/test/scala/integration/kafka/coordinator/transaction/ProducerIdsIntegrationTest.scala:
##
@@ -19,30 +19,30 @@ package kafka.coordinator.transaction
 
 import kafka.network.SocketServer
 import kafka.server.{IntegrationTestUtils, KafkaConfig}
+import kafka.test.ClusterInstance
 import kafka.test.annotation.{AutoStart, ClusterTest, ClusterTests, Type}
 import kafka.test.junit.ClusterTestExtensions
-import kafka.test.{ClusterConfig, ClusterInstance}
 import org.apache.kafka.common.message.InitProducerIdRequestData
 import org.apache.kafka.common.network.ListenerName
 import org.apache.kafka.common.protocol.Errors
 import org.apache.kafka.common.record.RecordBatch
 import org.apache.kafka.common.requests.{InitProducerIdRequest, 
InitProducerIdResponse}
 import org.apache.kafka.server.common.MetadataVersion
 import org.junit.jupiter.api.Assertions.{assertEquals, assertTrue}
-import org.junit.jupiter.api.{BeforeEach, Disabled, Timeout}
 import org.junit.jupiter.api.extension.ExtendWith
+import org.junit.jupiter.api.{BeforeEach, Disabled, Timeout}
 
 import java.util.stream.{Collectors, IntStream}
 import scala.concurrent.duration.DurationInt
 import scala.jdk.CollectionConverters._
 
 @ExtendWith(value = Array(classOf[ClusterTestExtensions]))
-class ProducerIdsIntegrationTest {
+class ProducerIdsIntegrationTest(private val cluster: ClusterInstance) {

Review Comment:
   Oh no problem, sorry I miss this one!



-- 
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-16482: Eliminate the IDE warnings of accepting ClusterConfig in BeforeEach [kafka]

2024-04-09 Thread via GitHub 


chia7712 commented on code in PR #15676:
URL: https://github.com/apache/kafka/pull/15676#discussion_r1558858994


##
core/src/test/scala/integration/kafka/coordinator/transaction/ProducerIdsIntegrationTest.scala:
##
@@ -19,30 +19,30 @@ package kafka.coordinator.transaction
 
 import kafka.network.SocketServer
 import kafka.server.{IntegrationTestUtils, KafkaConfig}
+import kafka.test.ClusterInstance
 import kafka.test.annotation.{AutoStart, ClusterTest, ClusterTests, Type}
 import kafka.test.junit.ClusterTestExtensions
-import kafka.test.{ClusterConfig, ClusterInstance}
 import org.apache.kafka.common.message.InitProducerIdRequestData
 import org.apache.kafka.common.network.ListenerName
 import org.apache.kafka.common.protocol.Errors
 import org.apache.kafka.common.record.RecordBatch
 import org.apache.kafka.common.requests.{InitProducerIdRequest, 
InitProducerIdResponse}
 import org.apache.kafka.server.common.MetadataVersion
 import org.junit.jupiter.api.Assertions.{assertEquals, assertTrue}
-import org.junit.jupiter.api.{BeforeEach, Disabled, Timeout}
 import org.junit.jupiter.api.extension.ExtendWith
+import org.junit.jupiter.api.{BeforeEach, Disabled, Timeout}
 
 import java.util.stream.{Collectors, IntStream}
 import scala.concurrent.duration.DurationInt
 import scala.jdk.CollectionConverters._
 
 @ExtendWith(value = Array(classOf[ClusterTestExtensions]))
-class ProducerIdsIntegrationTest {
+class ProducerIdsIntegrationTest(private val cluster: ClusterInstance) {

Review Comment:
   @KevinZTW Could you address this comment?



-- 
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-16482: Eliminate the IDE warnings of accepting ClusterConfig in BeforeEach [kafka]

2024-04-09 Thread via GitHub 


chia7712 commented on PR #15676:
URL: https://github.com/apache/kafka/pull/15676#issuecomment-2046467883

   @KevinZTW Could you try to use the new function to update this 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-16482: Eliminate the IDE warnings of accepting ClusterConfig in BeforeEach [kafka]

2024-04-09 Thread via GitHub 


chia7712 commented on PR #15676:
URL: https://github.com/apache/kafka/pull/15676#issuecomment-2045116605

   @KevinZTW Could you take a look at #15687? It offers a graceful way.


-- 
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-16482: Eliminate the IDE warnings of accepting ClusterConfig in BeforeEach [kafka]

2024-04-09 Thread via GitHub 


chia7712 commented on code in PR #15676:
URL: https://github.com/apache/kafka/pull/15676#discussion_r1557539944


##
core/src/test/scala/integration/kafka/coordinator/transaction/ProducerIdsIntegrationTest.scala:
##
@@ -19,30 +19,30 @@ package kafka.coordinator.transaction
 
 import kafka.network.SocketServer
 import kafka.server.{IntegrationTestUtils, KafkaConfig}
+import kafka.test.ClusterInstance
 import kafka.test.annotation.{AutoStart, ClusterTest, ClusterTests, Type}
 import kafka.test.junit.ClusterTestExtensions
-import kafka.test.{ClusterConfig, ClusterInstance}
 import org.apache.kafka.common.message.InitProducerIdRequestData
 import org.apache.kafka.common.network.ListenerName
 import org.apache.kafka.common.protocol.Errors
 import org.apache.kafka.common.record.RecordBatch
 import org.apache.kafka.common.requests.{InitProducerIdRequest, 
InitProducerIdResponse}
 import org.apache.kafka.server.common.MetadataVersion
 import org.junit.jupiter.api.Assertions.{assertEquals, assertTrue}
-import org.junit.jupiter.api.{BeforeEach, Disabled, Timeout}
 import org.junit.jupiter.api.extension.ExtendWith
+import org.junit.jupiter.api.{BeforeEach, Disabled, Timeout}
 
 import java.util.stream.{Collectors, IntStream}
 import scala.concurrent.duration.DurationInt
 import scala.jdk.CollectionConverters._
 
 @ExtendWith(value = Array(classOf[ClusterTestExtensions]))
-class ProducerIdsIntegrationTest {
+class ProducerIdsIntegrationTest(private val cluster: ClusterInstance) {

Review Comment:
   We can remove the cluster instance from all test cases now.



##
core/src/test/scala/integration/kafka/coordinator/transaction/ProducerIdsIntegrationTest.scala:
##
@@ -19,30 +19,30 @@ package kafka.coordinator.transaction
 
 import kafka.network.SocketServer
 import kafka.server.{IntegrationTestUtils, KafkaConfig}
+import kafka.test.ClusterInstance
 import kafka.test.annotation.{AutoStart, ClusterTest, ClusterTests, Type}
 import kafka.test.junit.ClusterTestExtensions
-import kafka.test.{ClusterConfig, ClusterInstance}
 import org.apache.kafka.common.message.InitProducerIdRequestData
 import org.apache.kafka.common.network.ListenerName
 import org.apache.kafka.common.protocol.Errors
 import org.apache.kafka.common.record.RecordBatch
 import org.apache.kafka.common.requests.{InitProducerIdRequest, 
InitProducerIdResponse}
 import org.apache.kafka.server.common.MetadataVersion
 import org.junit.jupiter.api.Assertions.{assertEquals, assertTrue}
-import org.junit.jupiter.api.{BeforeEach, Disabled, Timeout}
 import org.junit.jupiter.api.extension.ExtendWith
+import org.junit.jupiter.api.{BeforeEach, Disabled, Timeout}
 
 import java.util.stream.{Collectors, IntStream}
 import scala.concurrent.duration.DurationInt
 import scala.jdk.CollectionConverters._
 
 @ExtendWith(value = Array(classOf[ClusterTestExtensions]))
-class ProducerIdsIntegrationTest {
+class ProducerIdsIntegrationTest(private val cluster: ClusterInstance) {
 
   @BeforeEach
-  def setup(clusterConfig: ClusterConfig): Unit = {
-
clusterConfig.serverProperties().put(KafkaConfig.TransactionsTopicPartitionsProp,
 "1")
-
clusterConfig.serverProperties().put(KafkaConfig.TransactionsTopicReplicationFactorProp,
 "3")
+  def setUp(): Unit = {
+
cluster.config().serverProperties().put(KafkaConfig.TransactionsTopicPartitionsProp,
 "1")
+
cluster.config().serverProperties().put(KafkaConfig.TransactionsTopicReplicationFactorProp,
 "3")

Review Comment:
   We can remove this config as it is equal to default value



-- 
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-16482: Eliminate the IDE warnings of accepting ClusterConfig in BeforeEach [kafka]

2024-04-07 Thread via GitHub 


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

   
   
   *More detailed description of your change,
   if necessary. The PR title and PR message become
   the squashed commit message, so use a separate
   comment to ping reviewers.*
   
   *Summary of testing strategy (including rationale)
   for the feature or bug fix. Unit and/or integration
   tests are expected for any behaviour change and
   system tests should be considered for larger changes.*
   
   ### 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