You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "dongnuo123 (via GitHub)" <gi...@apache.org> on 2023/11/16 15:40:35 UTC
[PR] KAFKA-15756: [1/2] Migrate existing integration tests to run old protocol in new coordinator [kafka]
dongnuo123 opened a new pull request, #14781:
URL: https://github.com/apache/kafka/pull/14781
This pr
1. Adds genericGroupMaxSize and consumerGroupSessionTimeoutMs in the group coordinator config to GroupMetadataManager builder.
2. Adds framework to enable the new group coordinator in the integration test.
3. Migrates the following tests to enable the new group coordinator.
- AbstractConsumerTest
- ConsumerBounceTest
- BaseAsyncConsumerTest
- BaseConsumerTest
- PlaintextConsumerTest
- SaslMultiMechanismConsumerTest
### 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-15756: [1/2] Migrate existing integration tests to run old protocol in new coordinator [kafka]
Posted by "dongnuo123 (via GitHub)" <gi...@apache.org>.
dongnuo123 commented on code in PR #14781:
URL: https://github.com/apache/kafka/pull/14781#discussion_r1395936800
##########
core/src/test/scala/integration/kafka/api/ConsumerBounceTest.scala:
##########
@@ -77,8 +83,9 @@ class ConsumerBounceTest extends AbstractConsumerTest with Logging {
}
}
- @Test
- def testConsumptionWithBrokerFailures(): Unit = consumeWithBrokerFailures(10)
+ @ParameterizedTest(name = TestInfoUtils.TestWithParameterizedQuorumName)
+ @ValueSource(strings = Array("zk", "kraft", "kraft+kip848"))
+ def testConsumptionWithBrokerFailures(quorum: String): Unit = consumeWithBrokerFailures(10)
Review Comment:
This test is very flaky with kraft and kraft+kip848
--
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-15756: [1/2] Migrate existing integration tests to run old protocol in new coordinator [kafka]
Posted by "dongnuo123 (via GitHub)" <gi...@apache.org>.
dongnuo123 commented on code in PR #14781:
URL: https://github.com/apache/kafka/pull/14781#discussion_r1395938547
##########
core/src/test/scala/integration/kafka/api/PlaintextConsumerTest.scala:
##########
@@ -1805,16 +1865,19 @@ class PlaintextConsumerTest extends BaseConsumerTest {
s"The current assignment is ${consumer.assignment()}")
}
- @Test
- def testConsumingWithNullGroupId(): Unit = {
+ @ParameterizedTest(name = TestInfoUtils.TestWithParameterizedQuorumName)
+ @ValueSource(strings = Array("zk", "kraft", "kraft+kip848"))
+ def testConsumingWithNullGroupId(quorum: String): Unit = {
val topic = "test_topic"
val partition = 0;
val tp = new TopicPartition(topic, partition)
createTopic(topic, 1, 1)
- TestUtils.waitUntilTrue(() => {
- this.zkClient.topicExists(topic)
- }, "Failed to create topic")
+ if (!isKRaftTest()) {
+ TestUtils.waitUntilTrue(() => {
+ this.zkClientOrNull.topicExists(topic)
+ }, "Failed to create topic")
+ }
Review Comment:
I don't know if there's any alternative way to verify topic existence in KRaft but `topicExists` only works for zk.
--
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-15756: [1/2] Migrate existing integration tests to run old protocol in new coordinator [kafka]
Posted by "dongnuo123 (via GitHub)" <gi...@apache.org>.
dongnuo123 commented on code in PR #14781:
URL: https://github.com/apache/kafka/pull/14781#discussion_r1396159332
##########
core/src/test/scala/integration/kafka/api/PlaintextConsumerTest.scala:
##########
@@ -1805,16 +1865,19 @@ class PlaintextConsumerTest extends BaseConsumerTest {
s"The current assignment is ${consumer.assignment()}")
}
- @Test
- def testConsumingWithNullGroupId(): Unit = {
+ @ParameterizedTest(name = TestInfoUtils.TestWithParameterizedQuorumName)
+ @ValueSource(strings = Array("zk", "kraft", "kraft+kip848"))
+ def testConsumingWithNullGroupId(quorum: String): Unit = {
val topic = "test_topic"
val partition = 0;
val tp = new TopicPartition(topic, partition)
createTopic(topic, 1, 1)
- TestUtils.waitUntilTrue(() => {
- this.zkClient.topicExists(topic)
- }, "Failed to create topic")
+ if (!isKRaftTest()) {
+ TestUtils.waitUntilTrue(() => {
+ this.zkClientOrNull.topicExists(topic)
+ }, "Failed to create topic")
+ }
Review Comment:
Ah, we don't need to check if the topic exists for kraft. In `createTopic`, we already uses `TestUtils.createTopicWithAdmin` which contains checking topic existence.
```
if (isKRaftTest()) {
resource(createAdminClient(brokers, listenerName, adminClientConfig)) { admin =>
TestUtils.createTopicWithAdmin(
admin = admin,
topic = topic,
brokers = brokers,
controllers = controllerServers,
numPartitions = numPartitions,
replicationFactor = replicationFactor,
topicConfig = topicConfig
)
}
} ...
```
--
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-15756: [1/2] Migrate existing integration tests to run old protocol in new coordinator [kafka]
Posted by "dongnuo123 (via GitHub)" <gi...@apache.org>.
dongnuo123 commented on code in PR #14781:
URL: https://github.com/apache/kafka/pull/14781#discussion_r1396303939
##########
core/src/test/scala/integration/kafka/api/ConsumerBounceTest.scala:
##########
@@ -77,8 +83,9 @@ class ConsumerBounceTest extends AbstractConsumerTest with Logging {
}
}
- @Test
- def testConsumptionWithBrokerFailures(): Unit = consumeWithBrokerFailures(10)
+ @ParameterizedTest(name = TestInfoUtils.TestWithParameterizedQuorumName)
+ @ValueSource(strings = Array("zk", "kraft", "kraft+kip848"))
+ def testConsumptionWithBrokerFailures(quorum: String): Unit = consumeWithBrokerFailures(10)
Review Comment:
This is the only one flaky.
The test fails because somehow the consumer coordinator loses its generation (generation == null).
I guess it's related to timeout issue, because zk broker also fails in the same way when setting break points. Maybe something also needs time to propagate and reaches timeout before consumer coordinator updates the generation.
--
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-15756: [1/2] Migrate existing integration tests to run old protocol in new coordinator [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac merged PR #14781:
URL: https://github.com/apache/kafka/pull/14781
--
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-15756: [1/2] Migrate existing integration tests to run old protocol in new coordinator [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #14781:
URL: https://github.com/apache/kafka/pull/14781#discussion_r1396123024
##########
core/src/test/scala/integration/kafka/api/BaseAsyncConsumerTest.scala:
##########
@@ -28,8 +30,9 @@ import scala.jdk.CollectionConverters._
class BaseAsyncConsumerTest extends AbstractConsumerTest {
val defaultBlockingAPITimeoutMs = 1000
- @Test
- def testCommitAsync(): Unit = {
+ @ParameterizedTest(name = TestInfoUtils.TestWithParameterizedQuorumName)
+ @ValueSource(strings = Array("zk", "kraft", "kraft+kip848"))
Review Comment:
We don't need `zk` here.
##########
core/src/test/scala/integration/kafka/api/ConsumerBounceTest.scala:
##########
@@ -464,7 +477,7 @@ class ConsumerBounceTest extends AbstractConsumerTest with Logging {
private def submitCloseAndValidate(consumer: Consumer[Array[Byte], Array[Byte]],
closeTimeoutMs: Long, minCloseTimeMs: Option[Long], maxCloseTimeMs: Option[Long]): Future[Any] = {
executor.submit(() => {
- val closeGraceTimeMs = 2000
+ val closeGraceTimeMs = 10000
Review Comment:
I suppose that we increase it. However, it is weird that the cluster type has an impact here.
##########
core/src/test/scala/integration/kafka/api/PlaintextConsumerTest.scala:
##########
@@ -1805,16 +1865,19 @@ class PlaintextConsumerTest extends BaseConsumerTest {
s"The current assignment is ${consumer.assignment()}")
}
- @Test
- def testConsumingWithNullGroupId(): Unit = {
+ @ParameterizedTest(name = TestInfoUtils.TestWithParameterizedQuorumName)
+ @ValueSource(strings = Array("zk", "kraft", "kraft+kip848"))
+ def testConsumingWithNullGroupId(quorum: String): Unit = {
val topic = "test_topic"
val partition = 0;
val tp = new TopicPartition(topic, partition)
createTopic(topic, 1, 1)
- TestUtils.waitUntilTrue(() => {
- this.zkClient.topicExists(topic)
- }, "Failed to create topic")
+ if (!isKRaftTest()) {
+ TestUtils.waitUntilTrue(() => {
+ this.zkClientOrNull.topicExists(topic)
+ }, "Failed to create topic")
+ }
Review Comment:
We could perhaps use `TestUtils.createTopicWithAdmin`. I would take a look at how we migrated other tests to KRaft.
##########
core/src/test/scala/integration/kafka/api/ConsumerBounceTest.scala:
##########
@@ -77,8 +83,9 @@ class ConsumerBounceTest extends AbstractConsumerTest with Logging {
}
}
- @Test
- def testConsumptionWithBrokerFailures(): Unit = consumeWithBrokerFailures(10)
+ @ParameterizedTest(name = TestInfoUtils.TestWithParameterizedQuorumName)
+ @ValueSource(strings = Array("zk", "kraft", "kraft+kip848"))
+ def testConsumptionWithBrokerFailures(quorum: String): Unit = consumeWithBrokerFailures(10)
Review Comment:
Any idea why? Is it only this one or others are well?
##########
core/src/test/scala/integration/kafka/api/PlaintextConsumerTest.scala:
##########
@@ -547,7 +565,7 @@ class PlaintextConsumerTest extends BaseConsumerTest {
assertEquals(2, parts.size)
}
- @Test
+ @Test // TODO: doesn't pass for kraft and kraft+kip848
Review Comment:
I suppose that we have to wait on metadata propagation here.
##########
core/src/test/scala/integration/kafka/api/SaslMultiMechanismConsumerTest.scala:
##########
@@ -41,8 +43,9 @@ class SaslMultiMechanismConsumerTest extends BaseConsumerTest with SaslSetup {
closeSasl()
}
- @Test
- def testMultipleBrokerMechanisms(): Unit = {
+ @ParameterizedTest(name = TestInfoUtils.TestWithParameterizedQuorumName)
+ @ValueSource(strings = Array("zk", "kraft", "kraft+kip848"))
+ def testMultipleBrokerMechanisms(quorum: String): Unit = {
Review Comment:
Do we need to migrate it now or could we do it separately?
##########
core/src/test/scala/integration/kafka/api/BaseAsyncConsumerTest.scala:
##########
@@ -51,8 +54,9 @@ class BaseAsyncConsumerTest extends AbstractConsumerTest {
assertTrue(consumer.assignment.contains(tp))
}
- @Test
- def testCommitSync(): Unit = {
+ @ParameterizedTest(name = TestInfoUtils.TestWithParameterizedQuorumName)
+ @ValueSource(strings = Array("zk", "kraft", "kraft+kip848"))
Review Comment:
ditto.
--
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-15756: [1/2] Migrate existing integration tests to run old protocol in new coordinator [kafka]
Posted by "dongnuo123 (via GitHub)" <gi...@apache.org>.
dongnuo123 commented on code in PR #14781:
URL: https://github.com/apache/kafka/pull/14781#discussion_r1396303939
##########
core/src/test/scala/integration/kafka/api/ConsumerBounceTest.scala:
##########
@@ -77,8 +83,9 @@ class ConsumerBounceTest extends AbstractConsumerTest with Logging {
}
}
- @Test
- def testConsumptionWithBrokerFailures(): Unit = consumeWithBrokerFailures(10)
+ @ParameterizedTest(name = TestInfoUtils.TestWithParameterizedQuorumName)
+ @ValueSource(strings = Array("zk", "kraft", "kraft+kip848"))
+ def testConsumptionWithBrokerFailures(quorum: String): Unit = consumeWithBrokerFailures(10)
Review Comment:
This is the only one flaky.
The test fails because somehow the consumer coordinator loses its generation (generation == null).
I guess it's related to timeout issue, because zk broker also fails when setting break points. Maybe something also needs time to propagate and reaches timeout before consumer coordinator updates the generation.
--
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-15756: [1/2] Migrate existing integration tests to run old protocol in new coordinator [kafka]
Posted by "dongnuo123 (via GitHub)" <gi...@apache.org>.
dongnuo123 commented on code in PR #14781:
URL: https://github.com/apache/kafka/pull/14781#discussion_r1396303939
##########
core/src/test/scala/integration/kafka/api/ConsumerBounceTest.scala:
##########
@@ -77,8 +83,9 @@ class ConsumerBounceTest extends AbstractConsumerTest with Logging {
}
}
- @Test
- def testConsumptionWithBrokerFailures(): Unit = consumeWithBrokerFailures(10)
+ @ParameterizedTest(name = TestInfoUtils.TestWithParameterizedQuorumName)
+ @ValueSource(strings = Array("zk", "kraft", "kraft+kip848"))
+ def testConsumptionWithBrokerFailures(quorum: String): Unit = consumeWithBrokerFailures(10)
Review Comment:
This is the only one flaky.
The test fails because somehow the consumer coordinator loses its generation. I guess it's related to timeout issue, because zk broker also fails when setting break points. Maybe something also needs time to propagate and reaches timeout before consumer coordinator updates the generation.
--
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-15756: [1/2] Migrate existing integration tests to run old protocol in new coordinator [kafka]
Posted by "dongnuo123 (via GitHub)" <gi...@apache.org>.
dongnuo123 commented on code in PR #14781:
URL: https://github.com/apache/kafka/pull/14781#discussion_r1395939884
##########
core/src/test/scala/integration/kafka/api/SaslMultiMechanismConsumerTest.scala:
##########
@@ -41,8 +43,9 @@ class SaslMultiMechanismConsumerTest extends BaseConsumerTest with SaslSetup {
closeSasl()
}
- @Test
- def testMultipleBrokerMechanisms(): Unit = {
+ @ParameterizedTest(name = TestInfoUtils.TestWithParameterizedQuorumName)
+ @ValueSource(strings = Array("zk", "kraft", "kraft+kip848"))
+ def testMultipleBrokerMechanisms(quorum: String): Unit = {
Review Comment:
The test fails for kraft and kraft+kip848 with timeout error in createTopics in setup. The server repeatedly runs handleFetchRequest and causes the timeout. I don't know what could cause the loop...
--
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-15756: [1/2] Migrate existing integration tests to run old protocol in new coordinator [kafka]
Posted by "dongnuo123 (via GitHub)" <gi...@apache.org>.
dongnuo123 commented on code in PR #14781:
URL: https://github.com/apache/kafka/pull/14781#discussion_r1395938547
##########
core/src/test/scala/integration/kafka/api/PlaintextConsumerTest.scala:
##########
@@ -1805,16 +1865,19 @@ class PlaintextConsumerTest extends BaseConsumerTest {
s"The current assignment is ${consumer.assignment()}")
}
- @Test
- def testConsumingWithNullGroupId(): Unit = {
+ @ParameterizedTest(name = TestInfoUtils.TestWithParameterizedQuorumName)
+ @ValueSource(strings = Array("zk", "kraft", "kraft+kip848"))
+ def testConsumingWithNullGroupId(quorum: String): Unit = {
val topic = "test_topic"
val partition = 0;
val tp = new TopicPartition(topic, partition)
createTopic(topic, 1, 1)
- TestUtils.waitUntilTrue(() => {
- this.zkClient.topicExists(topic)
- }, "Failed to create topic")
+ if (!isKRaftTest()) {
+ TestUtils.waitUntilTrue(() => {
+ this.zkClientOrNull.topicExists(topic)
+ }, "Failed to create topic")
+ }
Review Comment:
I don't know if there's any alternative way to verify topic existence in KRaft.
--
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-15756: [1/2] Migrate existing integration tests to run old protocol in new coordinator [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #14781:
URL: https://github.com/apache/kafka/pull/14781#discussion_r1401788044
##########
core/src/test/scala/integration/kafka/api/IntegrationTestHarness.scala:
##########
@@ -67,6 +67,10 @@ abstract class IntegrationTestHarness extends KafkaServerTestHarness {
if (isZkMigrationTest()) {
cfgs.foreach(_.setProperty(KafkaConfig.MigrationEnabledProp, "true"))
}
+ if (isNewGroupCoordinatorEnabled()) {
+ cfgs.foreach(_.setProperty(KafkaConfig.UnstableApiVersionsEnableProp, "true"))
Review Comment:
We could actually remove this now that https://github.com/apache/kafka/commit/7826d5fc8ab695a5ad927338469ddc01b435a298 was merged.
##########
core/src/test/scala/integration/kafka/api/PlaintextConsumerTest.scala:
##########
@@ -547,23 +564,29 @@ class PlaintextConsumerTest extends BaseConsumerTest {
assertEquals(2, parts.size)
}
- @Test
- def testPartitionsForAutoCreate(): Unit = {
+ @ParameterizedTest(name = TestInfoUtils.TestWithParameterizedQuorumName)
+ @ValueSource(strings = Array("zk", "kraft", "kraft+kip848"))
+ def testPartitionsForAutoCreate(quorum: String): Unit = {
val consumer = createConsumer()
// First call would create the topic
consumer.partitionsFor("non-exist-topic")
- val partitions = consumer.partitionsFor("non-exist-topic")
- assertFalse(partitions.isEmpty)
+ var partitions = consumer.partitionsFor("non-exist-topic")
+ TestUtils.waitUntilTrue(() => {
+ partitions = consumer.partitionsFor("non-exist-topic")
+ !partitions.isEmpty
+ }, s"Timed out while awaiting non empty partitions.")
Review Comment:
nit: We don't really need `var partitions` here. How about?
```
TestUtils.waitUntilTrue(() => {
!consumer.partitionsFor("non-exist-topic").isEmpty
}, s"Timed out while awaiting non empty partitions.")
```
--
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-15756: [1/2] Migrate existing integration tests to run old protocol in new coordinator [kafka]
Posted by "dongnuo123 (via GitHub)" <gi...@apache.org>.
dongnuo123 commented on code in PR #14781:
URL: https://github.com/apache/kafka/pull/14781#discussion_r1395936049
##########
core/src/test/scala/integration/kafka/api/ConsumerBounceTest.scala:
##########
@@ -464,7 +477,7 @@ class ConsumerBounceTest extends AbstractConsumerTest with Logging {
private def submitCloseAndValidate(consumer: Consumer[Array[Byte], Array[Byte]],
closeTimeoutMs: Long, minCloseTimeMs: Option[Long], maxCloseTimeMs: Option[Long]): Future[Any] = {
executor.submit(() => {
- val closeGraceTimeMs = 2000
+ val closeGraceTimeMs = 10000
Review Comment:
Not sure if I can increase this. kraft and kraft+kip848 can't pass this test with 200 closeGraceTimeMs
--
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-15756: [1/2] Migrate existing integration tests to run old protocol in new coordinator [kafka]
Posted by "dongnuo123 (via GitHub)" <gi...@apache.org>.
dongnuo123 commented on code in PR #14781:
URL: https://github.com/apache/kafka/pull/14781#discussion_r1395938547
##########
core/src/test/scala/integration/kafka/api/PlaintextConsumerTest.scala:
##########
@@ -1805,16 +1865,19 @@ class PlaintextConsumerTest extends BaseConsumerTest {
s"The current assignment is ${consumer.assignment()}")
}
- @Test
- def testConsumingWithNullGroupId(): Unit = {
+ @ParameterizedTest(name = TestInfoUtils.TestWithParameterizedQuorumName)
+ @ValueSource(strings = Array("zk", "kraft", "kraft+kip848"))
+ def testConsumingWithNullGroupId(quorum: String): Unit = {
val topic = "test_topic"
val partition = 0;
val tp = new TopicPartition(topic, partition)
createTopic(topic, 1, 1)
- TestUtils.waitUntilTrue(() => {
- this.zkClient.topicExists(topic)
- }, "Failed to create topic")
+ if (!isKRaftTest()) {
+ TestUtils.waitUntilTrue(() => {
+ this.zkClientOrNull.topicExists(topic)
+ }, "Failed to create topic")
+ }
Review Comment:
I don't know if there's a corresponding way to verify topic existence in KRaft.
--
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-15756: [1/2] Migrate existing integration tests to run old protocol in new coordinator [kafka]
Posted by "dongnuo123 (via GitHub)" <gi...@apache.org>.
dongnuo123 commented on code in PR #14781:
URL: https://github.com/apache/kafka/pull/14781#discussion_r1395939129
##########
core/src/test/scala/integration/kafka/api/PlaintextConsumerTest.scala:
##########
@@ -547,7 +565,7 @@ class PlaintextConsumerTest extends BaseConsumerTest {
assertEquals(2, parts.size)
}
- @Test
+ @Test // TODO: doesn't pass for kraft and kraft+kip848
Review Comment:
This test doesn't pass for kraft and kraft+kip848. If we sleep for 5 sec between L572 and L573, the test passes. So I guess something is setting up or running in the background and the operation is not ready when L573 is called.
I didn't migrate this because it's not related to the group coordinator. Do we want to look into it further and migrate this as well?
--
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-15756: [1/2] Migrate existing integration tests to run old protocol in new coordinator [kafka]
Posted by "dongnuo123 (via GitHub)" <gi...@apache.org>.
dongnuo123 commented on code in PR #14781:
URL: https://github.com/apache/kafka/pull/14781#discussion_r1396137705
##########
core/src/test/scala/integration/kafka/api/PlaintextConsumerTest.scala:
##########
@@ -547,7 +565,7 @@ class PlaintextConsumerTest extends BaseConsumerTest {
assertEquals(2, parts.size)
}
- @Test
+ @Test // TODO: doesn't pass for kraft and kraft+kip848
Review Comment:
I can make it wait until receiving the partitions
--
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-15756: [1/2] Migrate existing integration tests to run old protocol in new coordinator [kafka]
Posted by "dongnuo123 (via GitHub)" <gi...@apache.org>.
dongnuo123 commented on code in PR #14781:
URL: https://github.com/apache/kafka/pull/14781#discussion_r1396161194
##########
core/src/test/scala/integration/kafka/api/SaslMultiMechanismConsumerTest.scala:
##########
@@ -41,8 +43,9 @@ class SaslMultiMechanismConsumerTest extends BaseConsumerTest with SaslSetup {
closeSasl()
}
- @Test
- def testMultipleBrokerMechanisms(): Unit = {
+ @ParameterizedTest(name = TestInfoUtils.TestWithParameterizedQuorumName)
+ @ValueSource(strings = Array("zk", "kraft", "kraft+kip848"))
+ def testMultipleBrokerMechanisms(quorum: String): Unit = {
Review Comment:
Let's do it separately
--
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