Re: [VOTE] KIP-467: Augment ProduceResponse error messaging

[Guozhang Wang <wa...@gmail.com>]

Hello folks,

Just a quick update on this KIP that, after we've started implementing it,
we realized it would be helpful to also add a few more broker-side metrics
for user visibility of different types of invalid record errors on log
validator.

The wiki page has been updated accordingly with the newly added metrics.


Guozhang


On Mon, Jul 29, 2019 at 9:31 AM Guozhang Wang <wa...@gmail.com> wrote:

> +1 from myself as well (binding).
>
>
> I'm closing this vote thread with the following votes:
>
> binding +1s: 4 (Guozhang, Jun, Jason, Bill).
>
>
> Thanks everyone who've reviewed and voted on the KIP!
>
> Guozhang
>
> On Mon, Jul 29, 2019 at 9:30 AM Guozhang Wang <wa...@gmail.com> wrote:
>
>> Yeah my thinking is that changing the return error code away from
>> CORRUPTED_RECORD is really a bug fix, so we should just do it anyways
>> without considering compatibility. I like returning INVALID_REQUEST too,
>> would change it in the wiki.
>>
>> Guozhang
>>
>> On Fri, Jul 26, 2019 at 4:40 PM Jason Gustafson <ja...@confluent.io>
>> wrote:
>>
>>> Hi Guozhang,
>>>
>>> I agree it is misleading to suggest corruption in these cases. Have you
>>> considered alternative error codes? I think INVALID_REQUEST may be more
>>> suggestive that the server has rejected the request for some reason.
>>>
>>> In any case, it's a small point that need not block the KIP. I'm +1
>>> overall.
>>>
>>> Thanks,
>>> Jason
>>>
>>> On Fri, Jul 26, 2019 at 4:24 PM Guozhang Wang <wa...@gmail.com>
>>> wrote:
>>>
>>> > Hi Jason, thanks for the comments.
>>> >
>>> > 1. Yes that's a good point. Will move it to `errors`.
>>> >
>>> > 2. The point is that when broker returning the new error code
>>> > INVALID_RECORD to the old versioned clients who do not recognize the
>>> code,
>>> > it would be translated to a UnknownServerException, whereas today
>>> (without
>>> > this KIP) the client would see CorruptRecordException that covers a
>>> bunch
>>> > of scenarios that actually are not related to corrupted records at all.
>>> >
>>> > I feel that the new behavior is actually better, i.e. let clients
>>> report an
>>> > UnknownServerException rather than a more concrete, but incorrect
>>> > CorruptRecordException. If we want to maintain compatibility we can let
>>> > brokers to return the same error code to old versioned clients, but
>>> I'm not
>>> > sure if it is actually better.
>>> >
>>> >
>>> > Guozhang
>>> >
>>> > On Thu, Jul 25, 2019 at 5:08 PM Jason Gustafson <ja...@confluent.io>
>>> > wrote:
>>> >
>>> > > Hi Guozhang,
>>> > >
>>> > > The proposal looks good. A couple minor questions.
>>> > >
>>> > > 1. InvalidRecordException is currently located in
>>> > > `org.apache.kafka.common.record`, which is not a public package.
>>> Shall we
>>> > > move it to `org.apache.kafka.common.errors`?
>>> > > 2. I'm not sure I understand the point about UnknownServerException
>>> in
>>> > the
>>> > > compatibility section. Are you suggesting that we would use the new
>>> error
>>> > > code even for old versions of the produce request?
>>> > >
>>> > > Thanks,
>>> > > Jason
>>> > >
>>> > >
>>> > >
>>> > >
>>> > > On Tue, Jul 16, 2019 at 3:46 PM Guozhang Wang <wa...@gmail.com>
>>> > wrote:
>>> > >
>>> > > > Hello folks,
>>> > > >
>>> > > > I'd like to start a voting thread on KIP-467 to improve error
>>> > > communication
>>> > > > and handling for producer response:
>>> > > >
>>> > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-467
>>> > > >
>>> >
>>> %3A+Augment+ProduceResponse+error+messaging+for+specific+culprit+records
>>> > > >
>>> > > >
>>> > > > Thanks,
>>> > > >
>>> > > > -- Guozhang
>>> > > >
>>> > >
>>> >
>>> >
>>> > --
>>> > -- Guozhang
>>> >
>>>
>>
>>
>> --
>> -- Guozhang
>>
>
>
> --
> -- Guozhang
>


-- 
-- Guozhang