[GitHub] sijie commented on a change in pull request #936: Issue 897: un-bind zookeeper from bookkeeper admin
sijie commented on a change in pull request #936: Issue 897: un-bind zookeeper from bookkeeper admin URL: https://github.com/apache/bookkeeper/pull/936#discussion_r160072577 ## File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeperAdmin.java ## @@ -1128,122 +1118,40 @@ public void processResult(int rc, String s, Object ctx) { *removed without prompt. * @return Returns true if format succeeds else false. */ -public static boolean format(ClientConfiguration conf, +public static boolean format(ServerConfiguration conf, boolean isInteractive, boolean force) throws Exception { -ZooKeeper zkc = ZooKeeperClient.newBuilder() -.connectString(conf.getZkServers()) -.sessionTimeoutMs(conf.getZkTimeout()) -.build(); -BookKeeper bkc = null; -try { -boolean ledgerRootExists = null != zkc.exists( -conf.getZkLedgersRootPath(), false); -boolean availableNodeExists = null != zkc.exists( -conf.getZkAvailableBookiesPath(), false); -List zkAcls = ZkUtils.getACLs(conf); -// Create ledgers root node if not exists -if (!ledgerRootExists) { -zkc.create(conf.getZkLedgersRootPath(), "".getBytes(UTF_8), -zkAcls, CreateMode.PERSISTENT); -} -// create available bookies node if not exists -if (!availableNodeExists) { -zkc.create(conf.getZkAvailableBookiesPath(), "".getBytes(UTF_8), -zkAcls, CreateMode.PERSISTENT); -} -// create readonly bookies node if not exists -if (null == zkc.exists(conf.getZkAvailableBookiesPath() + "/" + READONLY, false)) { -zkc.create( -conf.getZkAvailableBookiesPath() + "/" + READONLY, -new byte[0], -zkAcls, -CreateMode.PERSISTENT); -} +try (RegistrationManager rm = RegistrationManager.instantiateRegistrationManager(conf)) { +boolean ledgerRootExists = rm.prepareFormat(conf); // If old data was there then confirm with admin. +boolean doFormat = true; if (ledgerRootExists) { -boolean confirm = false; if (!isInteractive) { -// If non interactive and force is set, then delete old -// data. +// If non interactive and force is set, then delete old data. if (force) { -confirm = true; +doFormat = true; } else { -confirm = false; +doFormat = false; } } else { // Confirm with the admin. -confirm = IOUtils -.confirmPrompt("Ledger root already exists. " -+ "Are you sure to format bookkeeper metadata? " -+ "This may cause data loss."); -} -if (!confirm) { -LOG.error("BookKeeper metadata Format aborted!!"); -return false; -} -} -bkc = new BookKeeper(conf, zkc); -// Format all ledger metadata layout -bkc.ledgerManagerFactory.format(conf, zkc); - -// Clear underreplicated ledgers -try { -ZKUtil.deleteRecursive(zkc, ZkLedgerUnderreplicationManager.getBasePath(conf.getZkLedgersRootPath()) -+ BookKeeperConstants.DEFAULT_ZK_LEDGERS_ROOT_PATH); -} catch (KeeperException.NoNodeException e) { -if (LOG.isDebugEnabled()) { -LOG.debug("underreplicated ledgers root path node not exists in zookeeper to delete"); -} -} - -// Clear underreplicatedledger locks -try { -ZKUtil.deleteRecursive(zkc, ZkLedgerUnderreplicationManager.getBasePath(conf.getZkLedgersRootPath()) -+ '/' + BookKeeperConstants.UNDER_REPLICATION_LOCK); -} catch (KeeperException.NoNodeException e) { -if (LOG.isDebugEnabled()) { -LOG.debug("underreplicatedledger locks node not exists in zookeeper to delete"); -} -} - -// Clear the cookies -try { -ZKUtil.deleteRecursive(zkc, conf.getZkLedgersRootPath() -+ "/cookies"); -} catch (KeeperException.NoNodeException e) { -if (LOG.isDebugEnabled()) { -LOG.debug("cookies node not exists in zookeeper to delete"); +
[GitHub] sijie commented on a change in pull request #936: Issue 897: un-bind zookeeper from bookkeeper admin
sijie commented on a change in pull request #936: Issue 897: un-bind zookeeper from bookkeeper admin URL: https://github.com/apache/bookkeeper/pull/936#discussion_r160072057 ## File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java ## @@ -1512,13 +1470,10 @@ int runCmd(CommandLine cmdLine) throws Exception { printUsage(); return 1; } -ZooKeeper zk = null; -try { -zk = ZooKeeperClient.newBuilder() -.connectString(bkConf.getZkServers()) -.sessionTimeoutMs(bkConf.getZkTimeout()) -.build(); -LedgerManagerFactory mFactory = LedgerManagerFactory.newLedgerManagerFactory(bkConf, zk); + +try (LedgerManagerFactory mFactory = LedgerManagerFactory.newLedgerManagerFactory( +bkConf, + RegistrationManager.instantiateRegistrationManager(bkConf).getLayoutManager())) { Review comment: `RegistrationManager` will never be released. You need to do: ``` try (RegistrationManager rm = ...) { try (LedgerManagerFactory ...) { } } ``` This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] sijie commented on a change in pull request #936: Issue 897: un-bind zookeeper from bookkeeper admin
sijie commented on a change in pull request #936: Issue 897: un-bind zookeeper from bookkeeper admin URL: https://github.com/apache/bookkeeper/pull/936#discussion_r159357085 ## File path: bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/CompactionTest.java ## @@ -245,7 +245,14 @@ public void checkpointComplete(Checkpoint checkPoint, boolean compact) InterleavedLedgerStorage storage = new InterleavedLedgerStorage(); storage.initialize( conf, -LedgerManagerFactory.newLedgerManagerFactory(conf, zkc).newLedgerManager(), +LedgerManagerFactory +.newLedgerManagerFactory( +conf, +new ZkLayoutManager( Review comment: try to new layout manager from registration manager This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] sijie commented on a change in pull request #936: Issue 897: un-bind zookeeper from bookkeeper admin
sijie commented on a change in pull request #936: Issue 897: un-bind zookeeper from bookkeeper admin URL: https://github.com/apache/bookkeeper/pull/936#discussion_r159357040 ## File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/server/http/service/ListUnderReplicatedLedgerService.java ## @@ -51,12 +49,12 @@ static final Logger LOG = LoggerFactory.getLogger(ListUnderReplicatedLedgerService.class); protected ServerConfiguration conf; -protected ZooKeeper zk; +protected LayoutManager layoutManager; -public ListUnderReplicatedLedgerService(ServerConfiguration conf, ZooKeeper zk) { +public ListUnderReplicatedLedgerService(ServerConfiguration conf, LayoutManager layoutManager) { Review comment: Take LedgerManagerFactory from AutoRecovery This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] sijie commented on a change in pull request #936: Issue 897: un-bind zookeeper from bookkeeper admin
sijie commented on a change in pull request #936: Issue 897: un-bind zookeeper from bookkeeper admin URL: https://github.com/apache/bookkeeper/pull/936#discussion_r159356528 ## File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/DefaultSpeculativeRequestExecutionPolicy.java ## @@ -38,7 +38,7 @@ * are between {@code firstSpeculativeRequestTimeout} and {@code maxSpeculativeRequestTimeout}. */ public class DefaultSpeculativeRequestExecutionPolicy implements SpeculativeRequestExecutionPolicy { -private static final Logger LOG = LoggerFactory.getLogger(DefaultSpeculativeRequestExecutionPolicy.class); +private static final Logger LOG = LoggerFactory.getLogger(PendingReadOp.class); Review comment: I think this change is not needed. bad merge? This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] sijie commented on a change in pull request #936: Issue 897: un-bind zookeeper from bookkeeper admin
sijie commented on a change in pull request #936: Issue 897: un-bind zookeeper from bookkeeper admin URL: https://github.com/apache/bookkeeper/pull/936#discussion_r159356321 ## File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java ## @@ -512,9 +515,11 @@ private BookKeeper(ClientConfiguration conf, // initialize ledger manager try { this.ledgerManagerFactory = -LedgerManagerFactory.newLedgerManagerFactory(conf, ((ZKRegistrationClient) regClient).getZk()); +LedgerManagerFactory.newLedgerManagerFactory(conf, regClient.getLayoutManager()); } catch (KeeperException ke) { Review comment: if we remove zk, `LedgerManagerFactory.newLedgerManagerFactory` should not throw KeeperException. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] sijie commented on a change in pull request #936: Issue 897: un-bind zookeeper from bookkeeper admin
sijie commented on a change in pull request #936: Issue 897: un-bind zookeeper from bookkeeper admin URL: https://github.com/apache/bookkeeper/pull/936#discussion_r159356702 ## File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/AbstractZkLedgerManager.java ## @@ -322,9 +322,7 @@ public void processResult(int rc, String path, Object ctx) { @Override public void registerLedgerMetadataListener(long ledgerId, LedgerMetadataListener listener) { if (null != listener) { -if (LOG.isDebugEnabled()) { -LOG.debug("Registered ledger metadata listener {} on ledger {}.", listener, ledgerId); -} +LOG.debug("Registered ledger metadata listener {} on ledger {}.", listener, ledgerId); Review comment: bad merge here? This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] sijie commented on a change in pull request #936: Issue 897: un-bind zookeeper from bookkeeper admin
sijie commented on a change in pull request #936: Issue 897: un-bind zookeeper from bookkeeper admin URL: https://github.com/apache/bookkeeper/pull/936#discussion_r159356237 ## File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Journal.java ## @@ -459,8 +460,8 @@ public void run() { numReqInLastForceWrite = 0; } } +numReqInLastForceWrite += req.process(shouldForceWrite); Review comment: I think the changes in this file are not needed. probably come from merge? This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] sijie commented on a change in pull request #936: Issue 897: un-bind zookeeper from bookkeeper admin
sijie commented on a change in pull request #936: Issue 897: un-bind zookeeper from bookkeeper admin URL: https://github.com/apache/bookkeeper/pull/936#discussion_r159356917 ## File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/replication/Auditor.java ## @@ -134,7 +136,9 @@ private void initialize(ServerConfiguration conf, ZooKeeper zkc) throws UnavailableException { try { LedgerManagerFactory ledgerManagerFactory = LedgerManagerFactory Review comment: I would suggest getting layout manager from registration client, rather than construct zk layout manager directly. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] sijie commented on a change in pull request #936: Issue 897: un-bind zookeeper from bookkeeper admin
sijie commented on a change in pull request #936: Issue 897: un-bind zookeeper from bookkeeper admin URL: https://github.com/apache/bookkeeper/pull/936#discussion_r159356213 ## File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java ## @@ -842,15 +848,18 @@ public int runCmd(CommandLine cmdLine) throws Exception { return -1; } -ZooKeeper zk = null; LedgerManagerFactory mFactory = null; LedgerManager m = null; try { -zk = ZooKeeperClient.newBuilder() -.connectString(bkConf.getZkServers()) -.sessionTimeoutMs(bkConf.getZkTimeout()) -.build(); -mFactory = LedgerManagerFactory.newLedgerManagerFactory(bkConf, zk); +mFactory = LedgerManagerFactory.newLedgerManagerFactory( +bkConf, +new ZkLayoutManager( Review comment: try to avoid using ZkLayoutManager directly This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] sijie commented on a change in pull request #936: Issue 897: un-bind zookeeper from bookkeeper admin
sijie commented on a change in pull request #936: Issue 897: un-bind zookeeper from bookkeeper admin URL: https://github.com/apache/bookkeeper/pull/936#discussion_r159357018 ## File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/server/http/service/ListLedgerService.java ## @@ -53,12 +53,12 @@ static final Logger LOG = LoggerFactory.getLogger(ListLedgerService.class); protected ServerConfiguration conf; -protected ZooKeeper zk; +protected LayoutManager layoutManager; -public ListLedgerService(ServerConfiguration conf, ZooKeeper zk) { +public ListLedgerService(ServerConfiguration conf, LayoutManager layoutManager) { Review comment: This service should just take LedgerManagerFactory from bookie. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] sijie commented on a change in pull request #936: Issue 897: un-bind zookeeper from bookkeeper admin
sijie commented on a change in pull request #936: Issue 897: un-bind zookeeper from bookkeeper admin URL: https://github.com/apache/bookkeeper/pull/936#discussion_r159356554 ## File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/discover/RegistrationClient.java ## @@ -116,4 +116,7 @@ RegistrationClient initialize(ClientConfiguration conf, * @param listener listener to receive the topology changes of bookies. */ void unwatchReadOnlyBookies(RegistrationListener listener); + +// TODO: Review comment: remove TODO and add javadoc? This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] sijie commented on a change in pull request #936: Issue 897: un-bind zookeeper from bookkeeper admin
sijie commented on a change in pull request #936: Issue 897: un-bind zookeeper from bookkeeper admin URL: https://github.com/apache/bookkeeper/pull/936#discussion_r159356594 ## File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/discover/RegistrationManager.java ## @@ -104,4 +105,27 @@ RegistrationManager initialize(ServerConfiguration conf, * @throws BookieException when fail to remove cookie */ void removeCookie(String bookieId, Version version) throws BookieException; + +/** + * Gets layout manager. + * + * @return the layout manager + */ +LayoutManager getLayoutManager(); + +/** + * Prepare ledgers root node, availableNode, readonly node.. + * + * @param conf the conf + * @return Returns true if old data exists, false if not. + */ +boolean prepareFormat(ServerConfiguration conf) throws Exception; + +/** + * Do format boolean. + * + * @param conf the conf + * @return Returns true if success do format, false if not. + */ +boolean doFormat(ServerConfiguration conf) throws Exception; Review comment: rename this to `format`. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] sijie commented on a change in pull request #936: Issue 897: un-bind zookeeper from bookkeeper admin
sijie commented on a change in pull request #936: Issue 897: un-bind zookeeper from bookkeeper admin URL: https://github.com/apache/bookkeeper/pull/936#discussion_r159356972 ## File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/server/http/BKHttpServiceProvider.java ## @@ -79,6 +83,13 @@ private BKHttpServiceProvider(BookieServer bookieServer, this.bookieServer = bookieServer; this.autoRecovery = autoRecovery; this.serverConf = serverConf; +this.layoutManager = new ZkLayoutManager( Review comment: Can you get the layout manager from bookieServer? This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] sijie commented on a change in pull request #936: Issue 897: un-bind zookeeper from bookkeeper admin
sijie commented on a change in pull request #936: Issue 897: un-bind zookeeper from bookkeeper admin URL: https://github.com/apache/bookkeeper/pull/936#discussion_r159356478 ## File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeperAdmin.java ## @@ -1130,120 +1121,40 @@ public void processResult(int rc, String s, Object ctx) { */ public static boolean format(ClientConfiguration conf, boolean isInteractive, boolean force) throws Exception { -ZooKeeper zkc = ZooKeeperClient.newBuilder() -.connectString(conf.getZkServers()) -.sessionTimeoutMs(conf.getZkTimeout()) -.build(); -BookKeeper bkc = null; -try { -boolean ledgerRootExists = null != zkc.exists( -conf.getZkLedgersRootPath(), false); -boolean availableNodeExists = null != zkc.exists( -conf.getZkAvailableBookiesPath(), false); -List zkAcls = ZkUtils.getACLs(conf); -// Create ledgers root node if not exists -if (!ledgerRootExists) { -zkc.create(conf.getZkLedgersRootPath(), "".getBytes(UTF_8), -zkAcls, CreateMode.PERSISTENT); -} -// create available bookies node if not exists -if (!availableNodeExists) { -zkc.create(conf.getZkAvailableBookiesPath(), "".getBytes(UTF_8), -zkAcls, CreateMode.PERSISTENT); -} -// create readonly bookies node if not exists -if (null == zkc.exists(conf.getZkAvailableBookiesPath() + "/" + READONLY, false)) { -zkc.create( -conf.getZkAvailableBookiesPath() + "/" + READONLY, -new byte[0], -zkAcls, -CreateMode.PERSISTENT); -} +try (RegistrationManager rm = new ZKRegistrationManager()) { Review comment: Better load registration manager from configuration rather than constructing directly from ZKRegistrationManager? Change `format(ClientConfiguration ...)` to `format(ServerConfiguration ..)`, because registration manager is only available in ServerConfiguration. This format method is only called by BookieShell. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] sijie commented on a change in pull request #936: Issue 897: un-bind zookeeper from bookkeeper admin
sijie commented on a change in pull request #936: Issue 897: un-bind zookeeper from bookkeeper admin URL: https://github.com/apache/bookkeeper/pull/936#discussion_r159356203 ## File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java ## @@ -686,21 +688,25 @@ int runCmd(CommandLine cmdLine) throws Exception { predicate = replicasList -> !replicasList.contains(excludingBookieId); } -ZooKeeper zk = null; +LedgerManagerFactory mFactory = null; try { -zk = ZooKeeperClient.newBuilder() -.connectString(bkConf.getZkServers()) -.sessionTimeoutMs(bkConf.getZkTimeout()) -.build(); -LedgerManagerFactory mFactory = LedgerManagerFactory.newLedgerManagerFactory(bkConf, zk); +mFactory = LedgerManagerFactory.newLedgerManagerFactory( +bkConf, +new ZkLayoutManager( Review comment: I think it would be better to avoid construct ZkLayoutManager directly. A better way to do this: ``` try (RegistrationManager rm = ...) { try (LedgerManagerFactory mFactory = LedgerManagerFactory.newLedgerManagerFactory(.., rm.getLayoutManager()) { ... } } ``` This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] sijie commented on a change in pull request #936: Issue 897: un-bind zookeeper from bookkeeper admin
sijie commented on a change in pull request #936: Issue 897: un-bind zookeeper from bookkeeper admin URL: https://github.com/apache/bookkeeper/pull/936#discussion_r159357005 ## File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/server/http/service/GetLedgerMetaService.java ## @@ -45,12 +48,12 @@ static final Logger LOG = LoggerFactory.getLogger(GetLedgerMetaService.class); protected ServerConfiguration conf; -protected ZooKeeper zk; - -public GetLedgerMetaService(ServerConfiguration conf, ZooKeeper zk) { +protected LayoutManager layoutManager; +// TODO: remove zk? +public GetLedgerMetaService(ServerConfiguration conf, LayoutManager layoutManager) { Review comment: This service should just take LedgerManagerFactory from Bookie This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] sijie commented on a change in pull request #936: Issue 897: un-bind zookeeper from bookkeeper admin
sijie commented on a change in pull request #936: Issue 897: un-bind zookeeper from bookkeeper admin URL: https://github.com/apache/bookkeeper/pull/936#discussion_r159357105 ## File path: bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/EntryLogTest.java ## @@ -122,7 +120,6 @@ public void testMissingLogId() throws Exception { Bookie.checkDirectoryStructure(curDir); ServerConfiguration conf = TestBKConfiguration.newServerConfiguration(); -conf.setJournalDirName(tmpDir.toString()); Review comment: bad merge in this file? This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services