Issue 897: un-bind zookeeper from bookkeeper admin by hellostreaming · Pull Request #936 · apache/bookkeeper

hellostreaming · 2018-01-03T01:52:22Z

Descriptions of the changes in this PR:

remove zookeeper from LedgerLayout to make LedgerLayout as a pure layout object,
add LayoutManager, move zookeeper related utils into ZKLayoutManager for storing/reading/delete layout from zookeeper,
unbind zookeeper from LedgerManagerFactory, use LayoutManager instead,
unbind zookeeper from bookkeeperadmin.format,
fix compile and test errors.

Master Issue: #897

Be sure to do all of the following to help us incorporate your contribution
quickly and easily:

Make sure the PR title is formatted like:
<Issue # or BOOKKEEPER-#>: Description of pull request
e.g. Issue 123: Description ...
e.g. BOOKKEEPER-1234: Description ...

Make sure tests pass via mvn clean apache-rat:check install spotbugs:check.

Replace <Issue # or BOOKKEEPER-#> in the title with the actual Issue/JIRA number.

sijie

@jiazhai overall change looks good to me. left some comments. I think it would be better to avoid constructing ZkLayoutManager if possible. You should construct RegistrationManager from reflection and initializing ledger manager factory by using the layout manager from registration manager.

sijie · 2018-01-03T03:02:53Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java

-                LedgerManagerFactory mFactory = LedgerManagerFactory.newLedgerManagerFactory(bkConf, zk);
+                mFactory = LedgerManagerFactory.newLedgerManagerFactory(
+                    bkConf,
+                    new ZkLayoutManager(


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()) { ... } }

Thanks, will try it.

sijie · 2018-01-03T03:03:10Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java

-                mFactory = LedgerManagerFactory.newLedgerManagerFactory(bkConf, zk);
+                mFactory = LedgerManagerFactory.newLedgerManagerFactory(
+                    bkConf,
+                    new ZkLayoutManager(


try to avoid using ZkLayoutManager directly

Thanks, will change it.

sijie · 2018-01-03T03:03:43Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Journal.java

                                numReqInLastForceWrite = 0;
                            }
                        }
+                        numReqInLastForceWrite += req.process(shouldForceWrite);


I think the changes in this file are not needed. probably come from merge?

will change it, Seems it is from merge.

sijie · 2018-01-03T03:05:04Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java

            this.ledgerManagerFactory =
-                LedgerManagerFactory.newLedgerManagerFactory(conf, ((ZKRegistrationClient) regClient).getZk());
+                LedgerManagerFactory.newLedgerManagerFactory(conf, regClient.getLayoutManager());
        } catch (KeeperException ke) {


if we remove zk, LedgerManagerFactory.newLedgerManagerFactory should not throw KeeperException.

thanks. will change it.

right, will remove this.

sijie · 2018-01-03T03:07:44Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeperAdmin.java

-                    zkAcls,
-                    CreateMode.PERSISTENT);
-            }
+        try (RegistrationManager rm = new ZKRegistrationManager()) {


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.

Thanks, will change it.

sijie · 2018-01-03T03:18:47Z

...per-server/src/main/java/org/apache/bookkeeper/server/http/service/GetLedgerMetaService.java

-    public GetLedgerMetaService(ServerConfiguration conf, ZooKeeper zk) {
+    protected LayoutManager layoutManager;
+    // TODO: remove zk?
+    public GetLedgerMetaService(ServerConfiguration conf, LayoutManager layoutManager) {


This service should just take LedgerManagerFactory from Bookie

Thanks, will change it.

sijie · 2018-01-03T03:19:08Z

...keeper-server/src/main/java/org/apache/bookkeeper/server/http/service/ListLedgerService.java

+    protected LayoutManager layoutManager;

-    public ListLedgerService(ServerConfiguration conf, ZooKeeper zk) {
+    public ListLedgerService(ServerConfiguration conf, LayoutManager layoutManager) {


This service should just take LedgerManagerFactory from bookie.

sijie · 2018-01-03T03:19:33Z

...rc/main/java/org/apache/bookkeeper/server/http/service/ListUnderReplicatedLedgerService.java

+    protected LayoutManager layoutManager;

-    public ListUnderReplicatedLedgerService(ServerConfiguration conf, ZooKeeper zk) {
+    public ListUnderReplicatedLedgerService(ServerConfiguration conf, LayoutManager layoutManager) {


Take LedgerManagerFactory from AutoRecovery

Thanks, will change it.

sijie · 2018-01-03T03:20:23Z

bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/CompactionTest.java

+            LedgerManagerFactory
+                .newLedgerManagerFactory(
+                    conf,
+                    new ZkLayoutManager(


try to new layout manager from registration manager

Thanks, will change it.

sijie · 2018-01-03T03:20:42Z

bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/EntryLogTest.java

        Bookie.checkDirectoryStructure(curDir);

        ServerConfiguration conf = TestBKConfiguration.newServerConfiguration();
-        conf.setJournalDirName(tmpDir.toString());


bad merge in this file?

Thanks, will change it.

eolivelli

Nice work !

Left some comments

eolivelli · 2018-01-03T09:33:07Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/discover/ZKRegistrationClient.java

        return newBookieAddrs;
    }
+    @VisibleForTesting
+    public void setLayoutManager(LayoutManager layoutManager) {


Using powermock we can stop opening the internals to users with public methods

@sijie

eolivelli · 2018-01-03T15:47:15Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java

-                        .sessionTimeoutMs(bkConf.getZkTimeout())
-                        .build();
-                mFactory = LedgerManagerFactory.newLedgerManagerFactory(bkConf, zk);
+                mFactory = LedgerManagerFactory.newLedgerManagerFactory(


I think we are lacking some close().
You can considering even adding a close() method to LedgerManagerFactory and make it AutoCloseable in order to use try-with-resources

Yes, have thought do the changes. will do it.

eolivelli · 2018-01-03T15:50:17Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/discover/RegistrationManager.java

 @LimitedPrivate
 @Evolving
-public interface RegistrationManager extends AutoCloseable {
+public interface RegistrationManager extends LayoutManager, AutoCloseable {


Why a registration manager is a layoutmanager?
They are separate concepts, aren't they?

Thanks, will remove the extend here.

reddycharan · 2018-01-03T19:38:24Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeperAdmin.java

-            if (null != zkc) {
-                zkc.close();
-            }
+            return rm.format(conf);


it doesn't look appropriate to make 3 different calls here

rm.prepareFormat(conf)
bkc.ledgerManagerFactory.format(conf, bkc.regClient.getLayoutManager())
rm.format(conf)

is it not possible to abstract it out to single rm method call, probably with arguments?

thanks. ledger manager factory only manages ledger metadata, registration manager manages layout, cookies, instance id. so they are currently covering different metadata. so we need separate calls at this moment.
for end-user, they would still use the same format method, it doesn't change.

@reddycharan since this is a code/interface refactor, if you feel we can combine some some of the logics into one, how about do this in a separate PR? I think is better to keep an interface refactor simple rather than changing too much logic.

Hey @sijie, I'm working on having new set of bookieshellcommands - nukeexistingcluster and initnewcluster (by splitting metaformat. but leave metaformat as it is). Waiting for this commit to go in. Will consider refactoring while rebasing my commit on the new code, since it touches all these calls. Thanks.

cool. thanks @reddycharan

eolivelli

+1 now it looks great !

sijie

@jiazhai looks good now. left one comment there

sijie · 2018-01-08T03:35:56Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java

+
+            try (LedgerManagerFactory mFactory = LedgerManagerFactory.newLedgerManagerFactory(
+                    bkConf,
+                    RegistrationManager.instantiateRegistrationManager(bkConf).getLayoutManager())) {


RegistrationManager will never be released. You need to do:

try (RegistrationManager rm = ...) { try (LedgerManagerFactory ...) { } }

Thanks, leaked this change, will fix it.

…ctly

sijie reviewed Jan 3, 2018

View reviewed changes

hellostreaming force-pushed the issue_897 branch from 28fd782 to bc7eb03 Compare January 3, 2018 09:04

eolivelli requested changes Jan 3, 2018

View reviewed changes

reddycharan reviewed Jan 3, 2018

View reviewed changes

eolivelli approved these changes Jan 6, 2018

View reviewed changes

sijie approved these changes Jan 8, 2018

View reviewed changes

jiazhai added 6 commits January 9, 2018 13:12

un-bind zookeeper

da3fa6a

change following sijie's comments 1: fix merge issue

998164d

change following sijie's comments 2: avoid using ZkLayoutManager dire…

68308fe

…ctly

change following @eolivelli's commments

a200eb3

change following @eolivelli's commments

28e5f19

change following @sijie's comments

3c9c1c6

hellostreaming force-pushed the issue_897 branch from 2f29949 to 3c9c1c6 Compare January 9, 2018 05:18

sijie approved these changes Jan 9, 2018

View reviewed changes

fix test errors after merge

fc12408

jiazhai added this to the 4.7.0 milestone Jan 9, 2018

jiazhai added area/bookie area/client release/4.7.0 type/improvement labels Jan 9, 2018

jiazhai assigned hellostreaming Jan 9, 2018

jiazhai closed this in 998a55f Jan 9, 2018

Conversation

hellostreaming commented Jan 3, 2018

Uh oh!

sijie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

sijie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers