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
