+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 > > >
