Hi Jun, Thanks for the feedback. Using `Cluster` was appealing because it has the information we need and it is already a public class, so we would not need to introduce a new public class with a potentially confusing name. Having said that, I agree with your points and I have updated the KIP so that the listener is called ClusterResourceListener and we now pass a ClusterResource instance.
I have also clarified the motivation section a little and added a paragraph about the fact that the cluster id is only stored in ZooKeeper. I think this is fine for the first version and we can tackle improvements in future KIPs. I intend to start a vote soon as it seems like people are generally fine with the current proposal. Ismael On Wed, Aug 31, 2016 at 4:34 PM, Jun Rao <j...@confluent.io> wrote: > Ismael, > > Thanks for the proposal. It looks good overall. Just a comment below. > > We are adding the following interface in ClusterListener and Cluster > includes all brokers' endpoint and the metadata of topic partitions. > > void onClusterUpdate(Cluster cluster); > > > On the broker side, will that method be called when there is broker or > topic/partition change in metadata cache? Another thing is that Cluster > only includes one endpoint. This makes sense for the client. However, on > the broker side, it's not clear which endpoint should be used. > > In general, I am not sure how useful it is for serializers, interceptors, > metric reporters to know all brokers endpoints and topic/partition > metadata. > > I was thinking we could instead pass in sth like a ClusterResourceMeta and > just include the clusterId for now. This way, we can guarantee that > onClusterUpdate() > will only be called once and it's easier to implement on the broker side. > > Jun > > > On Wed, Aug 31, 2016 at 1:24 AM, Ismael Juma <ism...@juma.me.uk> wrote: > > > Thanks for the feedback Guozhang. Comment inline. > > > > On Wed, Aug 31, 2016 at 2:49 AM, Guozhang Wang <wangg...@gmail.com> > wrote: > > > > > About logging / debugging with the cluster id: I think the random UUID > > > itself may not be very helpful for human-readable debugging > information, > > > and we'd better use the cluster name mentioned in future work in > logging. > > > > > > > We can also add the human-readable value once it's available. However, > the > > random UUID is still useful now. After all, we use Git commit hashes in > > many places and they are significantly longer than what we are proposing > > here (40 instead of 22 characters) . When comparing by eye, one can often > > just look at the first few characters to distinguish. Does that make > sense? > > > > Ismael > > >
