search

apache / bookkeeper

Apache BookKeeper - a scalable, fault tolerant and low latency storage service optimized for append-only workloads
https://bookkeeper.apache.org/
Apache License 2.0
1.91k stars 905 forks source link

Improve bookie registration interface #621

Closed ivankelly closed 7 years ago

ivankelly commented 7 years ago

JIRA: https://issues.apache.org/jira/browse/BOOKKEEPER-628

Reporter: Rakesh R @rakeshadr

The idea is to improve/generalize the bookie registration process

Comments from JIRA

Rakesh R 2013-06-17T19:10:40.811+0000

I've created a patch to showcase the interfaces. Tries to generalize interfaces, these will be used by bkserver & bkclient.

Sijie Guo 2013-06-18T01:10:08.070+0000

thanks [~rakeshr]

since bookie registration involve zookeeper session expire handling, how you could make it generic?

general comments on your patch:

  1. please involve version in this interface. you could refere LedgerManager interface.
  2. this abstraction consider both client and server side change. so you need to ensure client and bookie agrees on using same implementation of this interface. check LedgerManagerFactory. it would be better to separate client-only, server-only and common part into different interfaces and managed by one factory.

Ivan Kelly 2013-06-18T10:48:54.353+0000

UnKnown should be Unknown. It's a single word.

This stuff has a good bit of overlap with the rack awareness stuff sijie did. Have you looked at that patch?

I agree with Sijie that something needs to be done about session expire handling. Perhaps register could take a callback, and you watch on the registered node. If it disappears, then you trigger the callback.

Generally I like this idea of this though.

Rakesh R 2013-06-23T20:01:15.542+0000

Thanks [~hustlmsp], [~ivank@yahoo-inc.com] for the reviews. I've separated out the client/server interfaces and if agrees, I'll integrate this to our code base.

bq.zookeeper session expire handling Presently ZooKeeperWatcherBase callback is doing the Expiry handling. I'm thinking to move the zookeeper instantiation to the RegistrationManager#init() method, and returns zk object. In bkclient, if user doesn't pass zk object will do the zk instantiation and returns the object? How does it sound?

Sijie Guo 2013-06-25T20:39:55.786+0000

{code} I'm thinking to move the zookeeper instantiation to the RegistrationManager#init() method, and returns zk object. {code}

in general, the interface is to allow different implementations. returning a zk object against its purpose. this interface should be something like HubServerManager interface in hedwig. https://github.com/apache/bookkeeper/blob/trunk/hedwig-server/src/main/java/org/apache/hedwig/server/topics/HubServerManager.java

Rakesh R 2013-07-03T10:00:39.487+0000

I've attached modified interfaces, added ManagerListener to listen for zk connection events. [~ikelly] [~hustlmsp] please have a look at this and if ok, will start implementing the idea.

Sijie Guo 2013-10-22T05:04:22.627+0000

the interface looks good.

but I would step back asking the question: what is the goal of this interface? it sounds to me that the interface is to hide zookeeper specific implementations. but from the interface, it is already tight with zookeeper (RegistrationManager#init(AbstractConfiguration conf, ZooKeeper zk, int managerVersion)).

As this interface doesn't block other tickets and all these stuffs (e.g. bookie registration, cookies) are quite zookeeper specific operations, I'd prefer generifying the interface after fixed zookeeper session handling in BOOKEEPER-537. so we could get clearer how the interface would look like. Thoughts?

Ivan Kelly 2013-10-22T08:26:54.868+0000

There's two reasons to provide an interface. Firstly to create well defined isolated contracts between the components of your system, and secondly to allow multiple implementations of this contract to coexist. In this case, we only want the first, as there's no driving usecase to move away from zookeeper for bookie registration. Following from this, RegistrationManagerFactory is unneeded. The interfaces in RegistrationManager are mostly fine though. I don't think that BookieRegistrationManager should have separate calls for the Cookie. bookieId <-> cookie is a 1-1 relationship. Why does ClientRegistrationManager have a createBookieInstance call? Also, the placement policy should have nothing to do with this interface.It's a decision that can be taken in total isolation on the client.

I agree with Sijie that it would be good to sort out the session handling before doing this work though. Or doing it in tandem. At least for the listener part.

Rakesh R 2013-10-23T04:45:52.978+0000

Thanks for the comments.

Here my idea was to unify the zk usage in bookie server operations, also in client side operations through one registration interface. Presently the zk apis usage are scattered in Bookie, Cookie. IMHO, this will be helpful to focus at common place for any ZK related logics like - ZK session handling, ZK acls.

bq.Why does ClientRegistrationManager have a createBookieInstance call? Yeah, this should be at server side, which will be used by bookie format logic.

Rakesh R 2013-10-23T04:45:53.091+0000

Thanks for the comments.

Here my idea was to unify the zk usage in bookie server operations, also in client side operations through one registration interface. Presently the zk apis usage are scattered in Bookie, Cookie. IMHO, this will be helpful to focus at common place for any ZK related logics like - ZK session handling, ZK acls.

bq.Why does ClientRegistrationManager have a createBookieInstance call? Yeah, this should be at server side, which will be used by bookie format logic.

Rakesh R 2013-10-23T04:49:08.806+0000

bq.I agree with Sijie that it would be good to sort out the session handling before doing this work though. Or doing it in tandem. At least for the listener part. I agree with both of you taking up this after pushing BOOKEEPER-537. [~hustlmsp] whats your opinion about pushing listener part separately with this JIRA.

Sijie Guo 2013-10-23T06:59:31.926+0000

[~rakeshr] [~ikelly] isn't ManagerListener related to zookeeper session handling? could we think of it after handling session expires?

Rakesh R 2013-10-23T10:48:49.704+0000

Yeah, zk session handling. Sure will revist after BOOKEEPER-537, I'will add dependency tag.

Ivan Kelly 2013-10-28T17:48:45.519+0000

Do you mean push the listener stuff in a different JIRA?

Rakesh R 2013-10-29T09:25:25.023+0000

I thought of pushing smaller chunk through subtask, but its overlapping with BOOKEEPER-537 and postponed my plans. Any thoughts?

Ivan Kelly 2013-10-30T11:29:45.331+0000

which smaller chunk though?

Rakesh R 2013-10-30T11:37:39.377+0000

bq.which smaller chunk though? I meant, RegistrationManager#ManagerListener part, which will be used to listen for zk connection events.

Ivan Kelly 2013-10-30T11:43:01.126+0000

Hmm, actually looking at this again, perhaps it could go in before BOOKKEEPER-537, if the previous comments are addressed. Then BOOKKEEPER-537 could build on top.

Sijie Guo 2013-10-30T17:51:52.386+0000

I don't think it is a good idea to refactor things that would be changed in future. since you don't actually know whether the refactor meets the changes or not. if this is really really need to be in, I would suggest doing the metadata related part (not session expire part), like Cookies. And we could iterate the session part later.

Sijie Guo 2013-11-10T22:28:55.214+0000

moved to 4.4.0 as session expire handling moved to 4.4.0

Ivan Kelly 2017-10-06T08:50:21.272+0000

[~zhaijia] Have you seen this? You're working on something similar now, no?

sijie commented 7 years ago

I think this is already done by #666 and #662 . Close and mark it as "duplicated"