eolivelli commented on a change in pull request #10715:
URL: https://github.com/apache/pulsar/pull/10715#discussion_r643043179
##########
File path:
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/BookiesApiTest.java
##########
@@ -30,23 +30,29 @@
import org.apache.pulsar.common.policies.data.BookieInfo;
import org.apache.pulsar.common.policies.data.BookiesClusterInfo;
import org.apache.pulsar.common.policies.data.BookiesRackConfiguration;
+import org.apache.pulsar.zookeeper.ZookeeperServerTest;
+import org.apache.zookeeper.server.ZooKeeperServer;
import org.testng.annotations.AfterMethod;
import org.testng.annotations.BeforeMethod;
import org.testng.annotations.Test;
@Slf4j
@Test(groups = "broker")
public class BookiesApiTest extends MockedPulsarServiceBaseTest {
+ private ZookeeperServerTest zks;
@BeforeMethod
@Override
public void setup() throws Exception {
+ zks = new ZookeeperServerTest(2181);
Review comment:
we should not use a fixed port
##########
File path:
pulsar-zookeeper-utils/src/test/java/org/apache/pulsar/zookeeper/ZookeeperServerTest.java
##########
@@ -41,18 +41,18 @@ public ZookeeperServerTest(int zkPort) throws IOException {
if (!zkTmpDir.delete() || !zkTmpDir.mkdir()) {
throw new IOException("Couldn't create zk directory " + zkTmpDir);
}
+ // Allow all commands on ZK control port
Review comment:
why are we anticipating the creation of these fields ?
##########
File path:
pulsar-zookeeper-utils/src/test/java/org/apache/pulsar/zookeeper/ZookeeperServerTest.java
##########
@@ -41,18 +41,18 @@ public ZookeeperServerTest(int zkPort) throws IOException {
if (!zkTmpDir.delete() || !zkTmpDir.mkdir()) {
throw new IOException("Couldn't create zk directory " + zkTmpDir);
}
+ // Allow all commands on ZK control port
+ System.setProperty("zookeeper.4lw.commands.whitelist", "*");
Review comment:
these are System properties, it is better to set them in a "static" block
##########
File path:
pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java
##########
@@ -626,7 +628,7 @@ public void start() throws PulsarServerException {
this.bkClientFactory = newBookKeeperClientFactory();
managedLedgerClientFactory = ManagedLedgerStorage.create(
- config, localMetadataStore, getZkClient(), bkClientFactory,
ioEventLoopGroup
+ config, localMetadataStore, createBookieZkClient(),
bkClientFactory, ioEventLoopGroup
Review comment:
Sorry, one last question.
who is going to `close` this ZooKeeper handle ?
when we use `getZkClient()` the ZooKeeper handle will be closed within the
PulsarService lifecycle.
but now we are going to create instances of it without never shutting them
down.
probably we have to store this reference to a field in PulsarService and
dispose it in the "close()" method
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
