Done. Thanks! On Thu, Apr 21, 2016 at 4:32 PM, Gwen Shapira <g...@confluent.io> wrote:
> Lets start a vote immediately? We are short of time toward the release. > > On Thu, Apr 21, 2016 at 2:57 PM, Ashish Singh <asi...@cloudera.com> wrote: > > Hey Guys, > > > > KIP-35 > > < > https://cwiki.apache.org/confluence/display/KAFKA/KIP-35+-+Retrieving+protocol+version > > > > has been updated based on latest discussions and following PRs have also > > been updated. > > 1. KAFKA-3307: Add ApiVersion request/response and server side handling. > > <https://github.com/apache/kafka/pull/986> > > 2. KAFKA-3600: Enhance java clients to use ApiVersion Req/Resp to check > if > > the broker they are talking to supports required api versions. > > <https://github.com/apache/kafka/pull/1251> > > > > If there are no major objections or changes suggested, we can start a > vote > > thread in a couple of days. > > > > On Tue, Apr 12, 2016 at 8:04 AM, Jun Rao <j...@confluent.io> wrote: > > > >> Hi, Ismael, > >> > >> The SASL engine that we used is the SASL library, right? How did the C > >> client generate those SASL tokens? Once a SASL mechanism is chosen, the > >> subsequent tokens are determined, right? So, my feeling is that those > >> tokens are part of SaslHandshakeRequest and are just extended across > >> multiple network packets. So modeling those as independent requests > feels > >> weird. When documentation them, we really need to document those as a > >> sequence, not individual isolated requests that can be issued > >> in arbitrary order. The version id will only add confusion since we > can't > >> version the tokens independently. We could explicitly add the client id > and > >> correlation id in the header, but I am not sure if it's worth the > >> additional complexity. > >> > >> Thanks, > >> > >> Jun > >> > >> On Tue, Apr 12, 2016 at 1:18 AM, Ismael Juma <ism...@juma.me.uk> wrote: > >> > >> > Hi Jun, > >> > > >> > I understand the point about the SASL tokens being similar to the SSL > >> > handshake in a way. However, is there any SASL library that handles > the > >> > network communication for these tokens? I couldn't find any and > without > >> > that, there isn't much benefit in deviating from Kafka's protocol (we > >> > basically save the space taken by the request header). It's worth > >> > mentioning that we are already adding the message size before the > opaque > >> > bytes provided by the library, so one could say we are already > extending > >> > the protocol. > >> > > >> > If we leave versioning aside, adding the standard Kafka request > header to > >> > those messages may also help from a debugging perspective as would > then > >> > include client id and correlation id along with the message. > >> > > >> > Ismael > >> > > >> > On Tue, Apr 12, 2016 at 2:13 AM, Jun Rao <j...@confluent.io> wrote: > >> > > >> > > Magnus, > >> > > > >> > > That sounds reasonable. To reduce the changes on the server side, > I'd > >> > > suggest the following minor tweaks on the proposal. > >> > > > >> > > 1. Continue supporting the separate SASL and SASL_SSL port. > >> > > > >> > > On SASL port, we support the new sequence > >> > > ApiVersionRequest (optional), SaslHandshakeRequest, SASL tokens, > >> > > regular > >> > > requests > >> > > > >> > > On SASL_SSL port, we support the new sequence > >> > > SSL handshake bytes, ApiVersionRequest (optional), > >> > > SaslHandshakeRequest, > >> > > SASL tokens, regular requests > >> > > > >> > > 2. We don't wrap SASL tokens in Kafka protocol. Similar to your > >> argument > >> > > about SSL handshake, those SASL tokens are generated by SASL library > >> and > >> > > Kafka doesn't really control its versioning. Kafka only controls the > >> > > selection of SASL mechanism, which will be versioned in > >> > > SaslHandshakeRequest. > >> > > > >> > > Does that work for you? > >> > > > >> > > Thanks, > >> > > > >> > > Jun > >> > > > >> > > > >> > > On Mon, Apr 11, 2016 at 11:15 AM, Magnus Edenhill < > mag...@edenhill.se> > >> > > wrote: > >> > > > >> > > > Hey Jun, see inline > >> > > > > >> > > > 2016-04-11 19:19 GMT+02:00 Jun Rao <j...@confluent.io>: > >> > > > > >> > > > > Hi, Magnus, > >> > > > > > >> > > > > Let me understand your proposal in more details just from the > >> > client's > >> > > > > perspective. My understanding of your proposal is the following. > >> > > > > > >> > > > > On plaintext port, the client will send the following bytes in > >> order. > >> > > > > ApiVersionRequest, SaslHandshakeRequest, SASL tokens (if > SASL > >> is > >> > > > > enabled), regular requests > >> > > > > > >> > > > > On SSL port, the client will send the following bytes in order. > >> > > > > SSL handshake bytes, ApiVersionRequest, > SaslHandshakeRequest, > >> > SASL > >> > > > > tokens (if SASL is enabled), regular requests > >> > > > > > >> > > > > >> > > > > >> > > > Yup! > >> > > > "SASL tokens" is a series of proper Kafka protocol > >> > SaslHandshakeRequests > >> > > > until > >> > > > the handshake is done. > >> > > > > >> > > > > >> > > > > >> > > > > > >> > > > > Is that right? Since we can use either SSL or SASL for > >> > authentication, > >> > > > it's > >> > > > > weird that in one case, we require ApiVersionRequest to happen > >> before > >> > > > > authentication and in another case we require the reverse. > >> > > > > > >> > > > > >> > > > Since the SSL/TLS is standardised and taken care of for us by the > SSL > >> > > > libraries it > >> > > > doesnt make sense to reimplement that on top of Kafka, so it isn't > >> > really > >> > > > comparable. > >> > > > But for SASL there is no standardised handshake protocol so we > must > >> > > either > >> > > > conceive one from scratch, or use the protocol that we already > have > >> > > > (Kafka). > >> > > > For the initial SASL implementation in 0.9 the first option was > >> chosen > >> > > and > >> > > > while > >> > > > it required a new protocol implementation in supporting clients > and > >> the > >> > > > broker > >> > > > it served its purpose. But not for long, it already needs to > evolve, > >> > > > and this gives us a golden[1] opportunity to make the > implementation > >> > > > reusable, evolvable, less complex > >> > > > and in line with all our other protocol work, by using the > protocol > >> > > stack > >> > > > of Kafka which the > >> > > > broker and all clients already have in place. > >> > > > > >> > > > Not taking this chance and instead diverging the custom SASL > >> handshake > >> > > > protocol > >> > > > even further from Kafka seems to me a weird choice. > >> > > > > >> > > > The current KIP-43 proposal does not have a clear compatibility > >> story; > >> > it > >> > > > doesnt seem to be possible > >> > > > to upgrade clients before brokers, while this might be okay for > the > >> > Java > >> > > > client, the KIP-35 discussion > >> > > > has hopefully proven that this assumption can't be made for the > >> entire > >> > > > eco-system. > >> > > > > >> > > > Let me be clear that there isn't anything technically wrong with > the > >> > > KIP-43 > >> > > > proposal (well, > >> > > > except for the hack to check byte[0] for 0x60 perhaps), but I'm > >> worried > >> > > the > >> > > > proposal will eventually lead to > >> > > > reimplementing Api Versioning, KIP-35, etc, in the custom SASL > >> > handshake, > >> > > > and this is just redundant, > >> > > > there is no technical reason for doing so and it'll just make > >> protocol > >> > > > semantics and implementations more complex. > >> > > > > >> > > > > >> > > > Regards, > >> > > > Magnus > >> > > > > >> > > > [1]: Timing is good for this change since only two clients, Java > and > >> C, > >> > > > currently supports > >> > > > the existing SASL handshake so far. > >> > > > > >> > > > > >> > > > > > >> > > > > Thanks, > >> > > > > > >> > > > > Jun > >> > > > > > >> > > > > On Mon, Apr 11, 2016 at 12:20 AM, Magnus Edenhill < > >> > mag...@edenhill.se> > >> > > > > wrote: > >> > > > > > >> > > > > > 2016-04-11 3:01 GMT+02:00 Jun Rao <j...@confluent.io>: > >> > > > > > > >> > > > > > > Thinking about ApiVersionRequest a bit more. There are > quite a > >> > few > >> > > > > things > >> > > > > > > special about it. In the ideal case, (1) its version should > >> never > >> > > > > change; > >> > > > > > > > >> > > > > > > >> > > > > > The only thing we know of the future is that we dont know > >> anything, > >> > > we > >> > > > > > can't > >> > > > > > think of every possible future use case, that's why need to be > >> able > >> > > to > >> > > > > > evolve interfaces > >> > > > > > as requirements and use-cases change. This is the gist of > KIP-35, > >> > and > >> > > > > > hampering > >> > > > > > KIP-35 itself, by not letting it also evolve, does not seem > right > >> > to > >> > > me > >> > > > > at > >> > > > > > all. > >> > > > > > > >> > > > > > > >> > > > > > > >> > > > > > > >> > > > > > > (2) it needs to be done before authentication (either > >> SSL/SASL); > >> > > (3) > >> > > > it > >> > > > > > is > >> > > > > > > required to be issued at the beginning of each connection > but > >> > never > >> > > > > needs > >> > > > > > > to be issued again on the same connection. So, instead of > >> > modeling > >> > > it > >> > > > > as > >> > > > > > a > >> > > > > > > regular request, it seems a cleaner approach is to just bake > >> that > >> > > > into > >> > > > > > the > >> > > > > > > initial connection handshake even before the authentication > >> > layer. > >> > > So > >> > > > > the > >> > > > > > > sequencing in a connection will be api discovery, > >> authentication, > >> > > > > > followed > >> > > > > > > by regular requests. I am not sure we can still add this in > a > >> > > > backward > >> > > > > > > compatible way now (e.g., not sure what the initial bytes > from > >> an > >> > > SSL > >> > > > > > > connection will look like). Even if we can do this in a > >> backward > >> > > > > > compatible > >> > > > > > > way, it's probably non-trivial amount of work though. > >> > > > > > > > >> > > > > > > >> > > > > > I have the luxory of not knowing the broker internals, so I > can > >> > only > >> > > > > > discuss > >> > > > > > this on a conceptual design level. > >> > > > > > > >> > > > > > In its simplest form each API request type has a NeedsAuth > flag > >> and > >> > > the > >> > > > > > broker protocol request layer simply checks if the current > >> session > >> > is > >> > > > > > Authenticated > >> > > > > > before processing the request: If not the session is closed > and > >> an > >> > > > error > >> > > > > is > >> > > > > > logged. > >> > > > > > The only two API requests that dont have the NeedsAuth flag > would > >> > be > >> > > > > > SaslHandshakeRequest > >> > > > > > and ApiVersionRequest, the latter could also use filtering to > >> only > >> > > > return > >> > > > > > the same two > >> > > > > > requests in ApiVersionResponse before the client is > authenticated > >> > (as > >> > > > not > >> > > > > > to "leak" information). > >> > > > > > If authentication is not configured on the broker all sessions > >> are > >> > > > deemed > >> > > > > > authenticated by default. > >> > > > > > > >> > > > > > > >> > > > > > Re backwards compatibility: > >> > > > > > My suggestion is to keep the current special SASL handshake > >> > protocol > >> > > on > >> > > > > the > >> > > > > > SASL_PLAIN/SASL_SSL > >> > > > > > endpoints, but use the new in-band Kafka SaslHandshakeRequest > API > >> > on > >> > > > the > >> > > > > > PLAIN/SSL endpoints. > >> > > > > > This way the broker is backwards compatible with older clients > >> that > >> > > > only > >> > > > > > supports the special SASL protocol, > >> > > > > > and newer cliets are also backwards compatible with older > brokers > >> > > that > >> > > > > only > >> > > > > > supports the special SASL protocol. > >> > > > > > Newer clients connecting to new brokers will be configured to > use > >> > > > > non-SASL > >> > > > > > ports and use the > >> > > > > > in-band Kafka SaslHandshakeRequest to authenticate. > >> > > > > > > >> > > > > > Using the existing standard Kafka protocol and the new > >> future-proof > >> > > > > > functionality of ApiVersionRequest > >> > > > > > allows the in-band authentication mechanisms and semantics to > >> > > naturally > >> > > > > > evolve over time > >> > > > > > without breaking existing clients. > >> > > > > > > >> > > > > > > >> > > > > > > >> > > > > > > >> > > > > > > >> > > > > > > > >> > > > > > > We started KIP-35 with supporting a client to know if a > version > >> > is > >> > > > > > > supported by the broker. It's now evolved into supporting a > >> > client > >> > > to > >> > > > > > > implement multiple versions of the protocol and dynamically > >> pick > >> > a > >> > > > > > version > >> > > > > > > supported by the broker. The former is likely solvable > without > >> > > > > > > ApiVersionRequest. How important is the latter? What if the > C > >> > > client > >> > > > > just > >> > > > > > > follows the java client model by implementing one version of > >> > > protocol > >> > > > > > per C > >> > > > > > > client release (which seems easier to implement)? > >> > > > > > > > >> > > > > > > >> > > > > > We've discussed this at length and it is not an option for > >> > > librdkafka, > >> > > > > nor > >> > > > > > kafka-python, and > >> > > > > > probably other clients as well, due to usability/UX and > >> maintenance > >> > > > > > reasons. > >> > > > > > (There's even discussion of making the Java client more > version > >> > > > > agnostic!) > >> > > > > > > >> > > > > > > >> > > > > > > >> > > > > > > >> > > > > > > >> > > > > > > > >> > > > > > > Thanks, > >> > > > > > > > >> > > > > > > Jun > >> > > > > > > > >> > > > > > > > >> > > > > > > On Fri, Apr 8, 2016 at 4:20 PM, Jun Rao <j...@confluent.io> > >> > wrote: > >> > > > > > > > >> > > > > > > > Magnus, > >> > > > > > > > > >> > > > > > > > A while back, we had another proposal for the broker to > just > >> > send > >> > > > the > >> > > > > > > > correlation id and an empty payload if it receives an > >> > unsupported > >> > > > > > version > >> > > > > > > > of the request. I didn't see that in the rejected > section. It > >> > > seems > >> > > > > > > simpler > >> > > > > > > > than the current proposal where the client has to issue an > >> > > > > > > > ApiVersionRequest on every connection. Could you document > the > >> > > > reason > >> > > > > > why > >> > > > > > > we > >> > > > > > > > rejected it? > >> > > > > > > > > >> > > > > > > > Thanks, > >> > > > > > > > > >> > > > > > > > Jun > >> > > > > > > > > >> > > > > > > > On Tue, Apr 5, 2016 at 1:47 PM, Ashish Singh < > >> > > asi...@cloudera.com> > >> > > > > > > wrote: > >> > > > > > > > > >> > > > > > > >> On Fri, Apr 1, 2016 at 1:32 AM, Ismael Juma < > >> > ism...@juma.me.uk> > >> > > > > > wrote: > >> > > > > > > >> > >> > > > > > > >> > Two more things: > >> > > > > > > >> > > >> > > > > > > >> > 3. We talk about backporting of new request versions to > >> > stable > >> > > > > > > branches > >> > > > > > > >> in > >> > > > > > > >> > the KIP. In practice, we can't do that until the Java > >> client > >> > > is > >> > > > > > > changed > >> > > > > > > >> so > >> > > > > > > >> > that it doesn't blindly use the latest protocol > version. > >> > > > > Otherwise, > >> > > > > > if > >> > > > > > > >> new > >> > > > > > > >> > request versions were added to 0.9.0.2, the client > would > >> > break > >> > > > > when > >> > > > > > > >> talking > >> > > > > > > >> > to a 0.9.0.1 broker (given Jason's proposal, it would > >> fail a > >> > > bit > >> > > > > > more > >> > > > > > > >> > gracefully, but that's still not good enough for a > stable > >> > > > branch). > >> > > > > > It > >> > > > > > > >> may > >> > > > > > > >> > be worth making this clear in the KIP (yes, it is a bit > >> > > > orthogonal > >> > > > > > and > >> > > > > > > >> > doesn't prevent the KIP from being adopted, but good to > >> > avoid > >> > > > > > > >> confusion). > >> > > > > > > >> > > >> > > > > > > >> Good point. Adding this note and also adding a note that > >> Kafka > >> > > has > >> > > > > not > >> > > > > > > >> backported an api version so far. > >> > > > > > > >> > >> > > > > > > >> > > >> > > > > > > >> > 4. The paragraph below is a bit confusing. It starts > >> talking > >> > > > about > >> > > > > > > 0.9.0 > >> > > > > > > >> > and trunk and then switches to 0.9.1. Is that > intentional? > >> > > > > > > >> > > >> > > > > > > >> Yes. > >> > > > > > > >> > >> > > > > > > >> > > >> > > > > > > >> > "Deprecation of a protocol version will be done by > >> marking a > >> > > > > > protocol > >> > > > > > > >> > version as deprecated in protocol documentation. > >> > Documentation > >> > > > > shall > >> > > > > > > >> also > >> > > > > > > >> > be used to indicate a protocol version that must not be > >> > used, > >> > > or > >> > > > > for > >> > > > > > > any > >> > > > > > > >> > such information.For instance, say 0.9.0 had protocol > >> > versions > >> > > > [0] > >> > > > > > for > >> > > > > > > >> api > >> > > > > > > >> > key 1. On trunk, version 1 of the api key was added. > Users > >> > > > running > >> > > > > > off > >> > > > > > > >> > trunk started using version 1 of the api and found out > a > >> > major > >> > > > > bug. > >> > > > > > To > >> > > > > > > >> > rectify that version 2 of the api is added to trunk. > For > >> > some > >> > > > > > reason, > >> > > > > > > >> it is > >> > > > > > > >> > now deemed important to have version 2 of the api in > 0.9.1 > >> > as > >> > > > > well. > >> > > > > > To > >> > > > > > > >> do > >> > > > > > > >> > so, version 1 and version 2 both of the api will be > >> > backported > >> > > > to > >> > > > > > the > >> > > > > > > >> 0.9.1 > >> > > > > > > >> > branch. 0.9.1 broker will return 0 as min supported > >> version > >> > > for > >> > > > > the > >> > > > > > > api > >> > > > > > > >> and > >> > > > > > > >> > 2 for the max supported version for the api. However, > the > >> > > > version > >> > > > > 1 > >> > > > > > > >> should > >> > > > > > > >> > be clearly marked as deprecated on its documentation. > It > >> > will > >> > > be > >> > > > > > > >> client's > >> > > > > > > >> > responsibility to make sure they are not using any such > >> > > > deprecated > >> > > > > > > >> version > >> > > > > > > >> > to the best knowledge of the client at the time of > >> > development > >> > > > (or > >> > > > > > > >> > alternatively by configuration)." > >> > > > > > > >> > > >> > > > > > > >> > Ismael > >> > > > > > > >> > > >> > > > > > > >> > > >> > > > > > > >> > > >> > > > > > > >> > On Fri, Apr 1, 2016 at 9:22 AM, Ismael Juma < > >> > > ism...@juma.me.uk> > >> > > > > > > wrote: > >> > > > > > > >> > > >> > > > > > > >> > > A couple of questions: > >> > > > > > > >> > > > >> > > > > > > >> > > 1. The KIP says "Specific version may be deprecated > >> > through > >> > > > > > protocol > >> > > > > > > >> > > documentation but must still be supported (although > it > >> is > >> > > fair > >> > > > > to > >> > > > > > > >> return > >> > > > > > > >> > an > >> > > > > > > >> > > error code if the specific API supports it).". It > may be > >> > > worth > >> > > > > > > >> expanding > >> > > > > > > >> > > this a little more. For example, what does it mean to > >> > > support > >> > > > > the > >> > > > > > > >> API? I > >> > > > > > > >> > > guess this means that the broker must not disconnect > the > >> > > > client > >> > > > > > and > >> > > > > > > >> the > >> > > > > > > >> > > broker must return a valid protocol response. Given > that > >> > it > >> > > > says > >> > > > > > > that > >> > > > > > > >> it > >> > > > > > > >> > is > >> > > > > > > >> > > "fair" (I would probably replace "fair" with > "valid") to > >> > > > return > >> > > > > an > >> > > > > > > >> error > >> > > > > > > >> > > code if the specific API supports it, it sounds like > we > >> > are > >> > > > > saying > >> > > > > > > >> that > >> > > > > > > >> > we > >> > > > > > > >> > > don't have to maintain the semantic behaviour (i.e. > we > >> > could > >> > > > > > > _always_ > >> > > > > > > >> > > return an error for a deprecated API?). Is this true? > >> > > > > > > >> > > > >> > > > > > > >> > > 2. ApiVersionQueryRequest seems a bit verbose, why > not > >> > > > > > > >> ApiVersionRequest? > >> > > > > > > >> > > > >> > > > > > > >> > > Ismael > >> > > > > > > >> > > > >> > > > > > > >> > > >> > > > > > > >> > >> > > > > > > >> > >> > > > > > > >> > >> > > > > > > >> -- > >> > > > > > > >> > >> > > > > > > >> Regards, > >> > > > > > > >> Ashish > >> > > > > > > >> > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > >> > > > > > > > > -- > > > > Regards, > > Ashish > -- Regards, Ashish
