Agree errors should be part of the protocol
> On Jan 15, 2015, at 17:59, Gwen Shapira <gshap...@cloudera.com> wrote:
>
> Hi,
>
> I got convinced by Joe and Dana that errors are indeed part of the
> protocol and can't be randomly added.
>
> So, it looks like we need to bump version of ProduceRequest in the
> following way:
> Version 0 -> accept acks >1. I think we should keep the existing
> behavior too (i.e. not replace it with -1) to avoid surprising
> clients, but I'm willing to hear other opinions.
> Version 1 -> do not accept acks >1 and return an error.
> Are we ok with the error I added in KAFKA-1697? We can use something
> less specific like InvalidRequestParameter. This error can be reused
> in the future and reduce the need to add errors, but will also be less
> clear to the client and its users. Maybe even add the error message
> string to the protocol in addition to the error code? (since we are
> bumping versions....)
>
> I think maintaining the old version throughout 0.8.X makes sense. IMO
> dropping it for 0.9 is feasible, but I'll let client owners help make
> that call.
>
> Am I missing anything? Should I start a KIP? It seems like a KIP-type
> discussion :)
>
> Gwen
>
>
> On Thu, Jan 15, 2015 at 2:31 PM, Ewen Cheslack-Postava
> <e...@confluent.io> wrote:
>> Gwen,
>>
>> I think the only option that wouldn't require a protocol version change is
>> the one where acks > 1 is converted to acks = -1 since it's the only one
>> that doesn't potentially break older clients. The protocol guide says that
>> the expected upgrade path is servers first, then clients, so old clients,
>> including non-java clients, that may be using acks > 1 should be able to
>> work with a new broker version.
>>
>> It's more work, but I think dealing with the protocol change is the right
>> thing to do since it eventually gets us to the behavior I think is better --
>> the broker should reject requests with invalid values. I think Joe and I
>> were basically in agreement. In my mind the major piece missing from his
>> description is how long we're going to maintain his "case 0" behavior. It's
>> impractical to maintain old versions forever, but it sounds like there
>> hasn't been a decision on how long to maintain them. Maybe that's another
>> item to add to KIPs -- protocol versions and behavior need to be listed as
>> deprecated and the earliest version in which they'll be removed should be
>> specified so users can understand which versions are guaranteed to be
>> compatible, even if they're using (well-written) non-java clients.
>>
>> -Ewen
>>
>>
>>> On Thu, Jan 15, 2015 at 12:52 PM, Dana Powers <dana.pow...@gmail.com> wrote:
>>>
>>>> clients don't break on unknown errors
>>>
>>> maybe true for the official java clients, but I dont think the assumption
>>> holds true for community-maintained clients and users of those clients.
>>> kafka-python generally follows the fail-fast philosophy and raises an
>>> exception on any unrecognized error code in any server response. in this
>>> case, kafka-python allows users to set their own required-acks policy when
>>> creating a producer instance. It is possible that users of kafka-python
>>> have deployed producer code that uses ack>1 -- perhaps in production
>>> environments -- and for those users the new error code will crash their
>>> producer code. I would not be surprised if the same were true of other
>>> community clients.
>>>
>>> *one reason for the fail-fast approach is that there isn't great
>>> documentation on what errors to expect for each request / response -- so
>>> we
>>> use failures to alert that some error case is not handled properly. and
>>> because of that, introducing new error cases without bumping the api
>>> version is likely to cause those errors to get raised/thrown all the way
>>> back up to the user. of course we (client maintainers) can fix the issues
>>> in the client libraries and suggest users upgrade, but it's not the ideal
>>> situation.
>>>
>>>
>>> long-winded way of saying: I agree w/ Joe.
>>>
>>> -Dana
>>>
>>>
>>> On Thu, Jan 15, 2015 at 12:07 PM, Gwen Shapira <gshap...@cloudera.com>
>>> wrote:
>>>
>>>> Is the protocol bump caused by the behavior change or the new error
>>>> code?
>>>>
>>>> 1) IMO, error_codes are data, and clients can expect to receive errors
>>>> that they don't understand (i.e. unknown errors). AFAIK, clients don't
>>>> break on unknown errors, they are simple more challenging to debug. If
>>>> we document the new behavior, then its definitely debuggable and
>>>> fixable.
>>>>
>>>> 2) The behavior change is basically a deprecation - i.e. acks > 1 were
>>>> never documented, and are not supported by Kafka clients starting with
>>>> version 0.8.2. I'm not sure this requires a protocol bump either,
>>>> although its a better case than new error codes.
>>>>
>>>> Thanks,
>>>> Gwen
>>>>
>>>> On Thu, Jan 15, 2015 at 10:10 AM, Joe Stein <joe.st...@stealth.ly>
>>>> wrote:
>>>>> Looping in the mailing list that the client developers live on because
>>>> they
>>>>> are all not on dev (though they should be if they want to be helping
>>>>> to
>>>>> build the best client libraries they can).
>>>>>
>>>>> I whole hardily believe that we need to not break existing
>>>>> functionality
>>>> of
>>>>> the client protocol, ever.
>>>>>
>>>>> There are many reasons for this and we have other threads on the
>>>>> mailing
>>>>> list where we are discussing that topic (no pun intended) that I don't
>>>> want
>>>>> to re-hash here.
>>>>>
>>>>> If we change wire protocol functionality OR the binary format (either)
>>>>> we
>>>>> must bump version AND treat version as a feature flag with backward
>>>>> compatibility support until it is deprecated for some time for folks
>>>>> to
>>>> deal
>>>>> with it.
>>>>>
>>>>> match version = {
>>>>> case 0: keepDoingWhatWeWereDoing()
>>>>> case 1: doNewStuff()
>>>>> case 2: doEvenMoreNewStuff()
>>>>> }
>>>>>
>>>>> has to be a practice we adopt imho ... I know feature flags can be
>>>> construed
>>>>> as "messy code" but I am eager to hear another (better? different?)
>>>> solution
>>>>> to this.
>>>>>
>>>>> If we don't do a feature flag like this specifically with this change
>>>> then
>>>>> what happens is that someone upgrades their brokers with a rolling
>>>> restart
>>>>> in 0.8.3 and every single one of their producer requests start to fail
>>>> and
>>>>> they have a major production outage. eeeek!!!!
>>>>>
>>>>> I do 100% agree that > 1 makes no sense and we *REALLY* need people to
>>>> start
>>>>> using 0,1,-1 but we need to-do that in a way that is going to work for
>>>>> everyone.
>>>>>
>>>>> Old producers and consumers must keep working with new brokers and if
>>>>> we
>>>> are
>>>>> not going to support that then I am unclear what the use of "version"
>>>>> is
>>>>> based on our original intentions of having it because of the
>>>>> 0.7=>-0.8.
>>>> We
>>>>> said no more breaking changes when we did that.
>>>>>
>>>>> - Joe Stein
>>>>>
>>>>> On Thu, Jan 15, 2015 at 12:38 PM, Ewen Cheslack-Postava <
>>>> e...@confluent.io>
>>>>> wrote:
>>>>>>
>>>>>> Right, so this looks like it could create an issue similar to what's
>>>>>> currently being discussed in
>>>>>> https://issues.apache.org/jira/browse/KAFKA-1649 where users now get
>>>>>> errors
>>>>>> under conditions when they previously wouldn't. Old clients won't
>>>>>> even
>>>>>> know
>>>>>> about the error code, so besides failing they won't even be able to
>>>>>> log
>>>>>> any
>>>>>> meaningful error messages.
>>>>>>
>>>>>> I think there are two options for compatibility:
>>>>>>
>>>>>> 1. An alternative change is to remove the ack > 1 code, but silently
>>>>>> "upgrade" requests with acks > 1 to acks = -1. This isn't the same as
>>>>>> other
>>>>>> changes to behavior since the interaction between the client and
>>>>>> server
>>>>>> remains the same, no error codes change, etc. The client might just
>>>>>> see
>>>>>> some increased latency since the message might need to be replicated
>>>>>> to
>>>>>> more brokers than they requested.
>>>>>> 2. Split this into two patches, one that bumps the protocol version
>>>>>> on
>>>>>> that
>>>>>> message to include the new error code but maintains both old (now
>>>>>> deprecated) and new behavior, then a second that would be applied in
>>>>>> a
>>>>>> later release that removes the old protocol + code for handling acks
>>>> 1.
>>>>>>
>>>>>> 2 is probably the right thing to do. If we specify the release when
>>>> we'll
>>>>>> remove the deprecated protocol at the time of deprecation it makes
>>>> things
>>>>>> a
>>>>>> lot easier for people writing non-java clients and could give users
>>>> better
>>>>>> predictability (e.g. if clients are at most 1 major release behind
>>>>>> brokers,
>>>>>> they'll remain compatible but possibly use deprecated features).
>>>>>>
>>>>>>
>>>>>> On Wed, Jan 14, 2015 at 3:51 PM, Gwen Shapira <gshap...@cloudera.com>
>>>>>> wrote:
>>>>>>
>>>>>>> Hi Kafka Devs,
>>>>>>>
>>>>>>> We are working on KAFKA-1697 - remove code related to ack>1 on the
>>>>>>> broker. Per Neha's suggestion, I'd like to give everyone a heads up
>>>>>>> on
>>>>>>> what these changes mean.
>>>>>>>
>>>>>>> Once this patch is included, any produce requests that include
>>>>>>> request.required.acks > 1 will result in an exception. This will be
>>>>>>> InvalidRequiredAcks in new versions (0.8.3 and up, I assume) and
>>>>>>> UnknownException in existing versions (sorry, but I can't add error
>>>>>>> codes retroactively).
>>>>>>>
>>>>>>> This behavior is already enforced by 0.8.2 producers (sync and
>>>>>>> new),
>>>>>>> but we expect impact on users with older producers that relied on
>>>>>>> acks
>>>>>>>> 1 and external clients (i.e python, go, etc).
>>>>>>>
>>>>>>> Users who relied on acks > 1 are expected to switch to using acks =
>>>>>>> -1
>>>>>>> and a min.isr parameter than matches their user case.
>>>>>>>
>>>>>>> This change was discussed in the past in the context of KAFKA-1555
>>>>>>> (min.isr), but let us know if you have any questions or concerns
>>>>>>> regarding this change.
>>>>>>>
>>>>>>> Gwen
>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> Thanks,
>>>>>> Ewen
>>>>>
>>>>>
>>>>> --
>>>>> You received this message because you are subscribed to the Google
>>>>> Groups
>>>>> "kafka-clients" group.
>>>>> To unsubscribe from this group and stop receiving emails from it, send
>>>>> an
>>>>> email to kafka-clients+unsubscr...@googlegroups.com.
>>>>> To post to this group, send email to kafka-clie...@googlegroups.com.
>>>>> Visit this group at http://groups.google.com/group/kafka-clients.
>>>>> To view this discussion on the web visit
>>>>
>>>> https://groups.google.com/d/msgid/kafka-clients/CAA7ooCBtH2JjyQsArdx_%3DV25B4O1QJk0YvOu9U6kYt9sB4aqng%40mail.gmail.com
>>>> .
>>>>>
>>>>> For more options, visit https://groups.google.com/d/optout.
>>>>
>>>> --
>>>> You received this message because you are subscribed to the Google
>>>> Groups
>>>> "kafka-clients" group.
>>>> To unsubscribe from this group and stop receiving emails from it, send
>>>> an
>>>> email to kafka-clients+unsubscr...@googlegroups.com.
>>>> To post to this group, send email to kafka-clie...@googlegroups.com.
>>>> Visit this group at http://groups.google.com/group/kafka-clients.
>>>> To view this discussion on the web visit
>>>>
>>>> https://groups.google.com/d/msgid/kafka-clients/CAHBV8WeUebxi%2B%2BSbjz8E9Yf4u4hkcPJ80Xsj0XTKcTac%3D%2B613A%40mail.gmail.com
>>>> .
>>>> For more options, visit https://groups.google.com/d/optout.
>>
>>
>>
>>
>> --
>> Thanks,
>> Ewen
