+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 <wangg...@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 <wangg...@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 <wangg...@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
