In general, I believe it is good to have design discussions in the
mailing list (do we want to modify the state machine? do we need to
maintain version per connection? which layer is responsible for the
API check)?
PR works well with tactical issues (class / object / method level) but
less so for design discussions.
The discussion doesn't need to be with the entire list (offlist
discussion with reviewers worked well in the past), but I noticed that
having at least one reviewer (not necessarily a committer - Jason
would be a fantastic reviewer here) who understands and agrees with
the design improves the discussion a lot.
Gwen
On Mon, Aug 15, 2016 at 8:27 AM, Ashish Singh <asi...@cloudera.com> wrote:
> Hello Dana,
>
> Thanks for the feedback.
>
> Kafka-3600 is not intended to address backwards compatibility of clients.
> Backwards compatibility of clients is a lengthier discussion and it will be
> better to take it on a separate KIP. We initiated that discussion on
> KIP-35's discuss thread and it was hard to reach a consensus there.
> However, much has changed since then and there are at least couple of
> non-java clients that support backwards compatibility and they do it
> differently. I too believe that having a consensus on what backwards
> compatibility in Kafka clients should mean and how to achieve it is very
> important. However, it is not intended to be tackled in this particular
> JIRA.
>
> As you mentioned, with KAFKA-3600 applications won't see no response and an
> abruptly closed socket when they try to talk to an unsupported broker
> version. Let me try list out concerns raised.
>
> 1. Is it worth the complexity just to add this check?
> Most of the work in the patch is to enhance NetworkClient to check api
> versions supported by brokers it has connections to. This information will
> be required by clients to enable features, as done by librdkafka, or to
> infer api version used by broker, IIUC as done by kafka-python.
>
> 2. Is a new connection state required?
> The new connection state helps in identifying what is a client doing right
> now. Having too many steps/ responsibilities in same step makes it hard to
> understand what a client is doing in a particular step or to debug. But, we
> can discuss more about this on PR.
>
> 3. Is network client required to maintain node api versions?
> Absolutely not. I figured feature enablement would require this following
> librdkafka's model. But I agree that as we have not yet decided on how to
> implement backwards compatibility, we should not put in any building block
> for a particular solution. I will be happy to take it out.
>
> Does above answers/ explanations address your concerns?
>
> On Fri, Aug 12, 2016 at 10:03 AM, Dana Powers <dana.pow...@gmail.com> wrote:
>
>> I think one of the hard parts is that while we agree that a call to
>> ApiVersions is required, I don't think there is agreement on how to
>> use the response.
>>
>> In kafka-python, for example, to support backwards compatibility we
>> set a single "api" configuration value during the client constructor.
>> This value is either set by the user explicitly via configuration or,
>> if unset by the user, inferred from the first bootstrap server that
>> responds. In either case we are choosing 1 "api" to apply to all
>> broker connections. We then use this configuration throughout the
>> client code to choose different feature paths (Zookeeper v. Kafka
>> offsets, for example; or whether to use group coordination apis v.
>> assign all partitions locally on 'subscribe'; etc).
>>
>> We do not short-circuit requests that appear unsupported by a
>> particular broker based on this api configuration. We send all
>> requests normally. If the broker doesn't understand a request, we
>> expect it to fail normally. Of course, "normally" here means no
>> response and an abruptly closed socket, which I think could be
>> improved.
>>
>> So KAFKA-3600. At its core, this revision implements a check on each
>> connection guaranteeing support for a configured api vector. If any
>> connection does not support that vector, the client raises a
>> KafkaException in the response handler.
>>
>> I think this is definitely an improvement. Compatibility errors are
>> completely opaque right now and can be very difficult to diagnose for
>> users that do not have direct access to their kafka cluster / brokers,
>> and/or do not have a firm grasp of the internals of the kafka client
>> and kafka protocol. But I also don't think this error handling is core
>> to compatibility. While supporting backwards compatibility, we
>> continue to rely on standard error handling (indeed, we have no choice
>> since we support backwards compatibility to 0.8.0).
>>
>> So I think the revision is attempting to add a few things that are
>> assumed to be building blocks for a compatibility layer:
>> (1) ApiVersions request / response handler;
>> (2) a new connection state,
>> (3) a hashmap of nodeApiVersions {node_id -> [api_versions]} .
>>
>> I'm confident that #1 is necessary. Is a new connection state
>> necessary? Is a full map of all nodes -> supported api version
>> necessary? These aren't used in the kafka-python implementation, so
>> from my perspective the answer is no. But maybe the java community has
>> some interesting ideas for improvement where these are key
>> ingredients. In any case, I think it would probably help substantially
>> to get some agreement on where the goalposts are for compatibility to
>> help push this forward.
>>
>> -Dana
>>
>> On Fri, Aug 12, 2016 at 7:37 AM, Ashish Singh <asi...@cloudera.com> wrote:
>> > Most of the work in the patch is to enhance NetworkClient to maintain api
>> > versions supported by brokers it has connections to. When a broker
>> > disconnects, its api versions info is removed and when it reconnects that
>> > info is fetched again. In short, with these changes Network Client at any
>> > given point knows what are the various brokers it is connected to and
>> what
>> > are the various apis they support. This information is required to
>> enable/
>> > select features based on brokers the client is talking to. I want to do
>> the
>> > following in the order mentioned.
>> >
>> > - Enable clients to detect, and so inform applications, that they are
>> > not compatible with brokers they are trying to talk to, rather than
>> getting
>> > connection dropped or similar not so sure response. This has been
>> ready for
>> > some time, so we can definitely work to include in next release.
>> > - Use the api versions info from NetworkClient to enable/ disable
>> > feature sets, something similar to what librdkafka already has. This
>> will
>> > need much broader testing.
>> >
>> > Note that both of the aforementioned steps have been discussed as part of
>> > KIP-35 discussions, but only first step was voted in. We will have to go
>> > through another KIP for second step.
>> >
>> > Any suggestions on how to make progress here will be helpful. Dana has
>> left
>> > some comments and I will address them soon. However, I would appreciate
>> if
>> > I can get a committer willing to help merging this in.
>> >
>> >
>> > On Thu, Aug 11, 2016 at 4:15 PM, Gwen Shapira <g...@confluent.io> wrote:
>> >
>> >> I am 100% pro smart Java clients that support KIP-35 and can use it to
>> >> work with newer brokers. If this JIRA makes sense as a step in that
>> >> direction, I think its great and remove my objection. I didn't see
>> >> anything that looked like a plan toward full forward-backward
>> >> compatibility, which is why I responded as I did...
>> >>
>> >> Verification is good, but it looked like there was much complexity
>> >> added toward very little benefits.
>> >>
>> >> On Thu, Aug 11, 2016 at 3:37 PM, Ashish Singh <asi...@cloudera.com>
>> wrote:
>> >> > Hey Gwen,
>> >> >
>> >> > I think this was more than a verification step, it was a building step
>> >> > towards a backwards compatible clients or for clients that can select
>> >> > feature based on brokers it is talking to. Are we now against the
>> idea of
>> >> > having smarter clients? This adds complexity to enable clients to
>> inform
>> >> > applications of incompatible broker versions, I think that is some
>> value
>> >> > add. In future, the api versions info can be used to take smarter
>> >> decisions.
>> >> >
>> >> > On Wed, Aug 10, 2016 at 11:04 PM, Gwen Shapira <g...@confluent.io>
>> >> wrote:
>> >> >
>> >> >> I hate doing this, because Ashish has really been good about
>> following
>> >> >> up on the PR, but I'm questioning the usefulness of this patch.
>> >> >>
>> >> >> It adds non-trivial complexity to the client... with not much return
>> >> >> on the investment, as far as I can see?
>> >> >> When I first suggested it, it was before KIP-35 was merged and
>> >> >> released and the intent was to validate KIP-35 (since I have low
>> >> >> opinion of protocols that aren't used). Since then KIP-35 was already
>> >> >> released, the followup turned more complex than we expected, I think.
>> >> >> And I'm wondering if it is worth it.
>> >> >>
>> >> >> The work and followup from Ashish is still super appreciated, but I
>> >> >> think we need more than appreciation - adding complexity to already
>> >> >> complex clients need to have functional justification...
>> >> >>
>> >> >> Anyway, I was out of the loop for ages, so feel free to yell at me
>> for
>> >> >> missing the obvious.
>> >> >>
>> >> >> Gwen
>> >> >>
>> >> >> On Tue, Aug 9, 2016 at 8:47 AM, Ashish Singh <asi...@cloudera.com>
>> >> wrote:
>> >> >> > Provided wrong link to PR, here is the PR
>> >> >> > <https://github.com/apache/kafka/pull/1251> for KAFKA-3600.
>> >> >> >
>> >> >> > On Tue, Aug 9, 2016 at 9:45 AM, Ashish Singh <asi...@cloudera.com>
>> >> >> wrote:
>> >> >> >
>> >> >> >> Hey Guys,
>> >> >> >>
>> >> >> >> KAFKA-3600 <https://issues.apache.org/jira/browse/KAFKA-3600> was
>> >> part
>> >> >> of
>> >> >> >> KIP-35's proposal. KAFKA-3307
>> >> >> >> <https://issues.apache.org/jira/browse/KAFKA-3307>,
>> >> >> >> adding ApiVersionsRequest/Response, was committed to 0.10.0.0, but
>> >> >> >> KAFKA-3600, enhancing java clients, is still under review. Here is
>> >> the
>> >> >> PR
>> >> >> >> <https://github.com/apache/kafka/pull/986>
>> >> >> >>
>> >> >> >> I have addressed all review comments and have been waiting for
>> >> further
>> >> >> >> reviews/ this to go in for quite some time. I will really
>> appreciate
>> >> if
>> >> >> a
>> >> >> >> committer can help with making progress on this.
>> >> >> >>
>> >> >> >> --
>> >> >> >>
>> >> >> >> Regards,
>> >> >> >> Ashish
>> >> >> >>
>> >> >> >
>> >> >> >
>> >> >> >
>> >> >> > --
>> >> >> >
>> >> >> > Regards,
>> >> >> > Ashish
>> >> >>
>> >> >>
>> >> >>
>> >> >> --
>> >> >> Gwen Shapira
>> >> >> Product Manager | Confluent
>> >> >> 650.450.2760 | @gwenshap
>> >> >> Follow us: Twitter | blog
>> >> >>
>> >> >
>> >> >
>> >> >
>> >> > --
>> >> >
>> >> > Regards,
>> >> > Ashish
>> >>
>> >>
>> >>
>> >> --
>> >> Gwen Shapira
>> >> Product Manager | Confluent
>> >> 650.450.2760 | @gwenshap
>> >> Follow us: Twitter | blog
>> >>
>> >
>> >
>> >
>> > --
>> >
>> > Regards,
>> > Ashish
>>
>
>
>
> --
>
> Regards,
> Ashish
--
Gwen Shapira
Product Manager | Confluent
650.450.2760 | @gwenshap
Follow us: Twitter | blog
