On Mon, Mar 14, 2016 at 9:37 AM, Gwen Shapira <g...@confluent.io> wrote:
> On Mon, Mar 14, 2016 at 7:05 AM, Ismael Juma <ism...@juma.me.uk> wrote: > > Hi Ashish, > > > > A few comments below. > > > > On Fri, Mar 11, 2016 at 9:59 PM, Ashish Singh <asi...@cloudera.com> > wrote: > > > >> Sounds like we are mostly in agreement. Following are the key points. > >> > >> 1. Every time a protocol version changes, for any request/response, > >> broker version, ApiVersion, will be bumped up. > >> > > > > Any thoughts on how we will enforce this? > > Code reviews? :) > > We are already doing it in ApiVersion (and have been since > 0.8.2.0-SNAPSHOT). Enforcing is awesome, but not necessarily part of > this KIP. > > > > > > >> 2. Protocol documentation will be versioned with broker version. > Every > >> time there is a broker version change, protocol documentation version > >> needs > >> to be updated and linked to main documentation page. > >> 3. Deprecation of protocol version will be done via marking the > version > >> as deprecated on the protocol documentation. > >> > > > > I think this is fine for the first version. We may consider doing more in > > the future (logging a warning perhaps). > > > > > >> 4. On getting unknown protocol version, broker will send an empty > >> response, instead of simply closing client connection. > >> > > > > I am not sure about this one. It's an unusual pattern and feels like a > hack. > > We discussed this and failed to come up with a better solution that > doesn't break compatibility. > Improvements can be added in the future - nothing can be worse than > current state (where the broker silently closes the connection) > > > > > 5. Metadata response will be enhanced to also contain broker version, > >> VersionInt and VersionString. VersionString will contain internal > >> version information. > >> > > > > Even though Magnus suggested that it's OK for clients to parse > > `VersionString`, I personally would rather avoid that. Do we really need > 3 > > separate versions or could we get away with 2? I think it would be good > to > > elaborate on this a bit and explain how each of these versions would be > > used (both from the broker and clients perspective). > > Agree! I'm also confused. > I am working on updating KIP and hopefully that will be less confusing. What I meant was metadata response will have broker-version, which will be made up of VersionInt and VersionString. For example, (4, "0.10.0-IV0"), this will be based on respective ApiVersions, https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/api/ApiVersion.scala#L100 . > > > > >> 6. Metadata request with single null topic and size of -1 can be > used to > >> fetch metadata response with only broker version information and no > >> topic/broker info. > > > > 7. On receiving a metadata request with single null topic with size of > >> -1, broker will respond with only broker version. > >> > > > > As Magnus says, the broker information should be returned. This would > also > > help us reduce unnecessary data transfer during NetworkClient's metadata > > updates (KAFKA-3358). At the moment, we get information for all topics in > > situations where we actually want no topics. > > > > Also, I think it's a bit odd to say a `single null topic with size -1`. > Do > > we mean an array of topics with size -1 and no elements? That would imply > > introducing a NULLABLE_ARRAY type (we currently have NULLABLE_STRING and > > NULLABLE_BYTES). > > > > Ismael > -- Regards, Ashish
