Hey guys, I haven't thought this all the way through, but I think having metadata in the NetworkClient actually does make sense. After all it is the case that all requests are directed to Kafka nodes so this is a higher level of abstraction to work at. I think the feature you are looking for is to avoid the automatic metadata update requests.
Wouldn't the simplest approach be to add a new parameter to the NetworkClient constructor (something like autoRefreshMetadata) and also add an updateMetadata(Cluster c) method for manual updates? Setting autoRefreshMetadata=false would disable to the metadata updates and depend on the user to manually update metadata. The existing usage would remain as in, but the client in the controller would do its own updates. I suppose this is just a question of whether or not to try to factor out the metadata update code. If we want to add another ClientNetworkClient layer I think that's reasonable but consider not making it a subsclass. These subclass/superclass interwinings can be a bit of a mess. -Jay On Tue, May 5, 2015 at 11:31 PM, Ewen Cheslack-Postava <e...@confluent.io> wrote: > +1 on trying to reuse the NetworkClient code. > > I think Jun's approach could work, but I'm wondering if refactoring a bit > could get better separation of concerns without a somewhat awkward nop > implementation of Metadata. I'm not sure what combination of delegation or > subclassing makes sense yet, but here's another approach that I think could > work: > > * Get rid of metadata stuff from NetworkClient. Add a subclass that also > manages all the metadata. (Since it's used for both producer and consumer, > the obvious name that I first jumped to is ClientNetworkClient, but somehow > I think we can come up with something less confusing.) > * leastLoadedNode is the only caller of metadata.fetch() in that class, > maybeUpdateMetadata is the only caller of leastLoadedNode, > maybeUpdateMetadata is only called in poll when a combination of metadata > related timeouts end up being 0. These can be safely refactored into the > subclass with one override of poll(). Same with metadataFetchInProgress > assuming the rest of the changes below. > * Some of the default implementations (e.g. handleMetadataResponse) can be > left nops in NetworkClient and moved to the subclass. > * Others can be overridden to call the super method then take the > additional action necessary (e.g., on disconnect, move the metadata update > request to the subclass). > * Making the timeout handling in poll() work for both NetworkClient and the > new base class might be the messiest part and might require breaking down > the implementation of poll into multiple methods. > * isReady uses metadataFetchInProgress and gets a timeout from the Metadata > class. We can just override this method as well, though I feel like there's > probably a cleaner solution. > > -Ewen > > > On Tue, May 5, 2015 at 4:54 PM, Jun Rao <j...@confluent.io> wrote: > > > Hi, Jiangjie, > > > > Thanks for taking on this. > > > > I was thinking that one way to decouple the dependency on Metadata in > > NetworkClient is the following. > > 1. Make Metadata an interface. > > 2. Rename current Metadata class to sth like KafkaMetadata that > implements > > the Metadata interface. > > 3. Have a new NoOpMetadata class that implements the Metadata interface. > > This class > > 3.1 does nothing for any write method > > 3.2 returns max long for any method that asks for a timestamp > > 3.3. returns an empty Cluster for fetch(). > > > > Then we can leave NetworkClient unchanged and just pass in a NoOpMetadata > > when using NetworkClient in the controller. The consumer/producer client > > will be using KafkaMetadata. > > > > As for replica fetchers, it may be possible to use KafkaConsumer. > However, > > we don't need the metadata and the offset management. So, perhaps it's > > easier to just use NetworkClient. Also, currently, there is one replica > > fetcher thread per source broker. By using NetworkClient, we can change > > that to using a single thread for all source brokers. This is probably a > > bigger change. So, maybe we can do it later. > > > > Jun > > > > > > I think we probably need to replace replica fetcher with NetworkClient as > > well. Replica fetcher gets leader from the controller and therefore > doesn't > > > > On Tue, May 5, 2015 at 1:37 PM, Jiangjie Qin <j...@linkedin.com.invalid> > > wrote: > > > > > I am trying to see if we can reuse the NetworkClient class to be used > in > > > controller to broker communication. (Also, we can probably use > > > KafkaConsumer which is already using NetworkClient in replica > fetchers). > > > Currently NetworkClient does the following things in addition to > sending > > > requests. > > > > > > 1. Connection state management. > > > 2. Flow control (inflight requests) > > > 3. Metadata refresh > > > > > > In controller we need (1) and (2) but not (3). NetworkClient is tightly > > > coupled with metadata now and this is the major blocker of reusing > > > NetworkClient in controller. For controller, we don’t need > NetworkClient > > > to manage any metadata because the controller has listeners to monitor > > the > > > cluster state and has all the information about topic metadata. > > > I am thinking we can add a disable metadata refresh flag to > NetworkClient > > > or set metadata refresh interval to be Long.MAX_VALUE, so the metadata > > will > > > be managed outside NetworkClient. > > > This needs minimal change to allow NetworkClient to be reused, but the > > > ugly part is NetworkClient still has the entire Metadata while it > > actually > > > only needs a NodeList. > > > > > > Want to see what do people think about this. > > > > > > Thanks. > > > > > > Jiangjie (Becket) Qin > > > > > > > > > > > > -- > Thanks, > Ewen >
