Ismael, thanks for the suggestions, added a bit more text to be more informative. Note that changes are made only to text and examples, the proposal remains as it was before voting was started. Magnus has already taken care of Rajini's comment.
KIP-35 has passed with +3 (binding) and +6 (non-binding). Thanks everyone for reviews and votes! On Fri, Apr 22, 2016 at 2:17 PM, Grant Henke <ghe...@cloudera.com> wrote: > +1 (non-binding) > On Apr 22, 2016 3:33 PM, "Magnus Edenhill" <mag...@edenhill.se> wrote: > > > Good point Rajini, I will clarify that. > > > > Thanks, > > Magnus > > > > 2016-04-22 12:35 GMT-07:00 Rajini Sivaram <rajinisiva...@googlemail.com > >: > > > > > +1 (non-binding) > > > > > > One minor comment: > > > > > > "11: The broker returns its full list of supported ApiKeys and versions > > > regardless of current authentication state (e.g., before SASL > > > authentication). If this is considered to leak information SSL can be > > used > > > for early authentication." > > > > > > > > > It may be better to explicitly state that ApiVersionRequest returns > > ApiKeys > > > and versions before SASL authentication (not regardless of current > > > authentication state) since you cannot send the request before SSL > client > > > authentication, and you cannot send the request during SASL > > authentication. > > > > > > > > > On Fri, Apr 22, 2016 at 7:47 PM, Ismael Juma <ism...@juma.me.uk> > wrote: > > > > > > > +1 (non-binding) from me, assuming that the changes suggested by Jun > > > below > > > > are included. > > > > > > > > Some minor comments. > > > > > > > > "5. Clients are recommended to use latest common supported API > > version." > > > > > > > > Maybe this would be clearer as: "Clients are recommended to use > latest > > > > version supported by the broker and itself." > > > > > > > > "6. 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)." > > > > > > > > I still think that the mention of both 0.9.0 and 0.9.1 is confusing > and > > > > it's not clear if we need it for this example. If we do need it, then > > we > > > > should define the state of 0.9.1 compared to 0.9.0 before we decide > to > > > > backport APIs. > > > > > > > > "11. The broker returns its full list of supported ApiKeys and > versions > > > > regardless of current authentication state (e.g., before SASL > > > > authentication). If this is considered to leak information SSL can be > > > used > > > > for early authentication." > > > > > > > > I think you mean "SSL with client authentication". > > > > > > > > Thanks, > > > > Ismael > > > > > > > > On Fri, Apr 22, 2016 at 11:31 AM, Jun Rao <j...@confluent.io> wrote: > > > > > > > > > Ashish, > > > > > > > > > > Just a couple of clarifications. > > > > > > > > > > 1. In ApiVersionRequest, we should get rid of ApiKeys since the > > request > > > > has > > > > > an empty body, right? > > > > > > > > > > 2. In ApiVersionResponse, we should list ErrorCode before > > ApiVersions, > > > > > right? > > > > > > > > > > Thanks, > > > > > > > > > > Jun > > > > > > > > > > On Thu, Apr 21, 2016 at 4:48 PM, Ashish Singh <asi...@cloudera.com > > > > > > wrote: > > > > > > > > > > > Hey Guys, > > > > > > > > > > > > I would like to re-initiate the voting process for *KIP-35: > > Retrieve > > > > > > protocol version*. > > > > > > > > > > > > KIP-35 can be accessed here > > > > > > < > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-35+-+Retrieving+protocol+version > > > > > > >. > > > > > > Following are a couple of related PRs. > > > > > > > > > > > > 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> > > > > > > > > > > > > The vote will run for 72 hours and I would like to start it with > my > > > +1 > > > > > > (non-binding). > > > > > > > > > > > > -- > > > > > > > > > > > > Regards, > > > > > > Ashish > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > Regards, > > > > > > Rajini > > > > > > -- Regards, Ashish
