mimaison commented on code in PR #15175:
URL: https://github.com/apache/kafka/pull/15175#discussion_r1638267850
##########
core/src/test/scala/integration/kafka/server/QuorumTestHarness.scala:
##########
@@ -366,6 +366,7 @@ abstract class QuorumTestHarness extends Logging {
props.setProperty(SocketServerConfigs.LISTENERS_CONFIG,
s"CONTROLLER://localhost:0")
props.setProperty(KRaftConfigs.CONTROLLER_LISTENER_NAMES_CONFIG,
"CONTROLLER")
props.setProperty(QuorumConfig.QUORUM_VOTERS_CONFIG,
s"$nodeId@localhost:0")
+ props.setProperty(ServerLogConfigs.LOG_DELETE_DELAY_MS_CONFIG, "1000")
Review Comment:
Why do we need to set this?
##########
core/src/test/scala/integration/kafka/api/SslAdminIntegrationTest.scala:
##########
@@ -42,6 +42,7 @@ object SslAdminIntegrationTest {
@volatile var lastUpdateRequestContext: Option[AuthorizableRequestContext] =
None
Review Comment:
Is there a reason we're not enabling KRaft on this file in this PR? I think
it would make sense to do it together with BaseAdminIntegrationTest,
SaslSslAdminIntegrationTest, especially since we're making changes to this file.
##########
core/src/test/scala/integration/kafka/api/SaslSslAdminIntegrationTest.scala:
##########
@@ -13,6 +13,7 @@
package kafka.api
import kafka.security.authorizer.AclAuthorizer
+import kafka.utils.TestInfoUtils
Review Comment:
This can be merged with the other imports on line 18
##########
core/src/test/scala/integration/kafka/api/SaslSslAdminIntegrationTest.scala:
##########
@@ -201,47 +216,66 @@ class SaslSslAdminIntegrationTest extends
BaseAdminIntegrationTest with SaslSetu
// Delete only ACLs on literal 'mytopic2' topic
var deleted =
client.deleteAcls(List(acl2.toFilter).asJava).all().get().asScala.toSet
- assertEquals(Set(acl2), deleted)
+ brokers.foreach { b =>
+ TestUtils.waitAndVerifyRemovedAcl(acl2.entry(),
b.dataPlaneRequestProcessor.authorizer.get, acl2.pattern())
+ }
assertEquals(Set(anyAcl, fooAcl, prefixAcl), getAcls(allTopicAcls))
ensureAcls(deleted)
// Delete only ACLs on literal '*' topic
deleted =
client.deleteAcls(List(anyAcl.toFilter).asJava).all().get().asScala.toSet
- assertEquals(Set(anyAcl), deleted)
+ brokers.foreach { b =>
+ TestUtils.waitAndVerifyRemovedAcl(anyAcl.entry(),
b.dataPlaneRequestProcessor.authorizer.get, anyAcl.pattern())
+ }
assertEquals(Set(acl2, fooAcl, prefixAcl), getAcls(allTopicAcls))
ensureAcls(deleted)
// Delete only ACLs on specific prefixed 'mytopic' topics:
deleted =
client.deleteAcls(List(prefixAcl.toFilter).asJava).all().get().asScala.toSet
- assertEquals(Set(prefixAcl), deleted)
+ brokers.foreach { b =>
+ TestUtils.waitAndVerifyRemovedAcl(prefixAcl.entry(),
b.dataPlaneRequestProcessor.authorizer.get, prefixAcl.pattern())
+ }
assertEquals(Set(anyAcl, acl2, fooAcl), getAcls(allTopicAcls))
ensureAcls(deleted)
// Delete all literal ACLs:
deleted =
client.deleteAcls(List(allLiteralTopicAcls).asJava).all().get().asScala.toSet
- assertEquals(Set(anyAcl, acl2, fooAcl), deleted)
+ brokers.foreach { b =>
+ TestUtils.waitAndVerifyRemovedAcl(anyAcl.entry(),
b.dataPlaneRequestProcessor.authorizer.get, anyAcl.pattern())
Review Comment:
Instead of repeating the block, can we loop through the ACLs we expect to be
deleted? Something like:
```
Set(anyAcl, acl2, fooAcl).foreach { acl =>
TestUtils.waitAndVerifyRemovedAcl(acl.entry(),
b.dataPlaneRequestProcessor.authorizer.get, acl.pattern())
}
```
Same below.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
