Hi Sarwar, It seems like we didn't reach a consensus last time about this KIP. So I would suggest revising the KIP, and then creating a new DISCUSS thread when it's ready for review.
regards, Colin On Wed, Apr 14, 2021, at 01:26, Sarwar Bhuiyan wrote: > Forgive me while I find my feet, but since this reached a VOTE stage, is it > really required to go through a DISCUSS thread again or can we continue to > address the issues on this thread since the issues were raised on this > thread? If I do have to create a new DISCUSS thread, do I just copy the > messages from the old thread to it to address them? > > Thank you. > > Sarwar > > On Wed, Apr 14, 2021 at 9:18 AM Stanislav Kozlovski <stanis...@confluent.io> > wrote: > > > Hey all, > > > > To revive this old KIP, Sarwar Bhuiyan has volunteered to take over > > ownership. > > He will continue to drive this KIP to approval and completion - I > > understand that he will re-start the discussion with a new [DISCUSS] or > > [VOTE] thread. > > > > Thank you Sarwar! > > > > Best, > > Stanislav > > > > On Fri, Jan 10, 2020 at 5:55 PM Gwen Shapira <g...@confluent.io> wrote: > > > >> Sorry for the super late reply, but since the vote thread showed up, > >> I've read the KIP and have a concern. > >> The concern was raised by Matthias Sax earlier, but I didn't see it > >> addressed. > >> > >> Basically the current iteration of the KIP unifies deserialization > >> errors with corruption. As was pointed out, these are not the same > >> thing. Corruption means that the message itself (including envelope, > >> not just the payload) is broken. De-serialization errors mean that > >> either key or value serializers have a problem. It can even be a > >> temporary problem of connecting to schema registry, I believe. Corrupt > >> messages can only be skipped. De-serialization errors can (and > >> arguably should) be retried with a different serializer. Something > >> like Connect will need to skip corrupt messages, but messages with > >> SerDe issues should probably go into a dead-letter queue. > >> > >> Anyway, IMO we need exceptions that will let us tell the difference. > >> > >> Gwen > >> > >> On Fri, Oct 11, 2019 at 10:05 AM Stanislav Kozlovski > >> <stanis...@confluent.io> wrote: > >> > > >> > Thanks Jason. I've edited the KIP with the latest proposal. > >> > > >> > On Thu, Oct 10, 2019 at 2:00 AM Jason Gustafson <ja...@confluent.io> > >> wrote: > >> > > >> > > Hi Stanislav, > >> > > > >> > > Sorry for the late comment. I'm happy with the current proposal. Just > >> one > >> > > small request is to include an example which shows how a user could > >> use > >> > > this to skip over a record. > >> > > > >> > > I'd suggest pushing this to a vote to see if anyone else has feedback. > >> > > > >> > > Thanks, > >> > > Jason > >> > > > >> > > On Sat, Jul 13, 2019 at 2:27 PM Stanislav Kozlovski < > >> > > stanis...@confluent.io> > >> > > wrote: > >> > > > >> > > > Hello, > >> > > > > >> > > > Could we restart the discussion here again? > >> > > > > >> > > > My last message was sent on the 3rd of June but I haven't received > >> > > replies > >> > > > since then. > >> > > > > >> > > > I'd like to get this KIP to a finished state, regardless of whether > >> that > >> > > is > >> > > > merged-in or discarded. It has been almost one year since the > >> publication > >> > > > of the KIP. > >> > > > > >> > > > Thanks, > >> > > > Stanislav > >> > > > > >> > > > On Mon, Jun 3, 2019 at 11:19 AM Stanislav Kozlovski < > >> > > > stanis...@confluent.io> > >> > > > wrote: > >> > > > > >> > > > > Do people agree with the approach I outlined in my last reply? > >> > > > > > >> > > > > On Mon, May 6, 2019 at 2:12 PM Stanislav Kozlovski < > >> > > > stanis...@confluent.io> > >> > > > > wrote: > >> > > > > > >> > > > >> Hey there Kamal, > >> > > > >> > >> > > > >> I'm sincerely sorry for missing your earlier message. As I open > >> this > >> > > > >> thread up, I see I have an unsent draft message about resuming > >> > > > discussion > >> > > > >> from some time ago. > >> > > > >> > >> > > > >> In retrospect, I think I may have been too pedantic with the > >> exception > >> > > > >> naming and hierarchy. > >> > > > >> I now believe a single exception type of > >> > > > `RecordDeserializationException` > >> > > > >> is enough. Let's go with that. > >> > > > >> > >> > > > >> On Mon, May 6, 2019 at 6:40 AM Kamal Chandraprakash < > >> > > > >> kamal.chandraprak...@gmail.com> wrote: > >> > > > >> > >> > > > >>> Matthias, > >> > > > >>> > >> > > > >>> We already have CorruptRecordException which doesn't extend the > >> > > > >>> SerializationException. So, we need an alternate > >> > > > >>> name suggestion for the corrupted record error if we decide to > >> extend > >> > > > the > >> > > > >>> FaultyRecordException class. > >> > > > >>> > >> > > > >>> Stanislav, > >> > > > >>> > >> > > > >>> Our users are also facing this error. Could we bump up this > >> > > discussion? > >> > > > >>> > >> > > > >>> I think we can have a single exception type > >> > > > >>> FaultyRecordException/RecordDeserialization exception to > >> capture both > >> > > > >>> the errors. We can add an additional enum field to > >> differentiate the > >> > > > >>> errors > >> > > > >>> if required. > >> > > > >>> > >> > > > >>> Thanks, > >> > > > >>> Kamal Chandraprakash > >> > > > >>> > >> > > > >>> On Wed, Apr 24, 2019 at 1:49 PM Kamal Chandraprakash < > >> > > > >>> kamal.chandraprak...@gmail.com> wrote: > >> > > > >>> > >> > > > >>> > Stanislav, > >> > > > >>> > > >> > > > >>> > Any updates on this KIP? We have internal users who want to > >> skip > >> > > the > >> > > > >>> > corrupted message while consuming the records. > >> > > > >>> > > >> > > > >>> > > >> > > > >>> > On Fri, Oct 19, 2018 at 11:34 PM Matthias J. Sax < > >> > > > >>> matth...@confluent.io> > >> > > > >>> > wrote: > >> > > > >>> > > >> > > > >>> >> I am not 100% familiar with the details of the consumer code, > >> > > > however > >> > > > >>> I > >> > > > >>> >> tend to disagree with: > >> > > > >>> >> > >> > > > >>> >> > There's no difference between the two cases -- if (and > >> only if) > >> > > > the > >> > > > >>> >> message is corrupt, it can't be deserialized. If (and only > >> if) it > >> > > > >>> can't be > >> > > > >>> >> deserialized, it is corrupt. > >> > > > >>> >> > >> > > > >>> >> Assume that a user configures a JSON deserializer but a > >> faulty > >> > > > >>> upstream > >> > > > >>> >> producer writes an Avro message. For this case, the message > >> is not > >> > > > >>> >> corrupted, but still can't be deserialized. And I can > >> imaging that > >> > > > >>> users > >> > > > >>> >> want to handle both cases differently. > >> > > > >>> >> > >> > > > >>> >> Thus, I think it makes sense to have two different exceptions > >> > > > >>> >> `RecordDeserializationException` and > >> `CorruptedRecordException` > >> > > that > >> > > > >>> can > >> > > > >>> >> both extend `FaultyRecordException` (don't like this name > >> too much > >> > > > >>> >> honestly, but don't have a better idea for it anyway). > >> > > > >>> >> > >> > > > >>> >> Side remark. If we introduce class > >> > > `RecordDeserializationException` > >> > > > >>> and > >> > > > >>> >> `CorruptedRecordException`, we can also add an interface > >> that both > >> > > > >>> >> implement to return partition/offset information and let both > >> > > extend > >> > > > >>> >> `SerializationException` directly without an intermediate > >> class in > >> > > > the > >> > > > >>> >> exception hierarchy. > >> > > > >>> >> > >> > > > >>> >> > >> > > > >>> >> -Matthias > >> > > > >>> >> > >> > > > >>> >> On 8/8/18 2:57 AM, Stanislav Kozlovski wrote: > >> > > > >>> >> >> If you are inheriting from SerializationException, your > >> derived > >> > > > >>> class > >> > > > >>> >> > should also be a kind of serialization exception. Not > >> something > >> > > > >>> more > >> > > > >>> >> > general. > >> > > > >>> >> > Yeah, the reason for inheriting it would be for > >> > > > >>> backwards-compatibility. > >> > > > >>> >> > > >> > > > >>> >> >> Hmm. Can you think of any new scenarios that would make > >> Kafka > >> > > > >>> force > >> > > > >>> >> the > >> > > > >>> >> > user need to skip a specific record? Perhaps one scenario > >> is if > >> > > > >>> records > >> > > > >>> >> > are lost but we don't know how many. > >> > > > >>> >> > Not on the spot, but I do wonder how likely a new scenario > >> is to > >> > > > >>> >> surface in > >> > > > >>> >> > the future and how we'd handle the exceptions' class > >> hierarchy > >> > > > then. > >> > > > >>> >> > > >> > > > >>> >> >> Which offset were we planning to use in the > >> > > > >>> >> > exception? > >> > > > >>> >> > The offset of the record which caused the exception. In > >> the case > >> > > > of > >> > > > >>> >> > batches, we use the last offset of the batch. In both > >> cases, > >> > > users > >> > > > >>> >> should > >> > > > >>> >> > have to seek +1 from the given offset. You can review the > >> PR to > >> > > > >>> ensure > >> > > > >>> >> its > >> > > > >>> >> > accurate > >> > > > >>> >> > > >> > > > >>> >> > > >> > > > >>> >> > If both of you prefer `RecordDeserializationException`, we > >> can > >> > > go > >> > > > >>> with > >> > > > >>> >> > that. Please do confirm that is okay > >> > > > >>> >> > > >> > > > >>> >> > On Tue, Aug 7, 2018 at 11:35 PM Jason Gustafson < > >> > > > ja...@confluent.io > >> > > > >>> > > >> > > > >>> >> wrote: > >> > > > >>> >> > > >> > > > >>> >> >> One difference between the two cases is that we can't > >> generally > >> > > > >>> trust > >> > > > >>> >> the > >> > > > >>> >> >> offset of a corrupt message. Which offset were we > >> planning to > >> > > use > >> > > > >>> in > >> > > > >>> >> the > >> > > > >>> >> >> exception? Maybe it should be either the fetch offset or > >> one > >> > > plus > >> > > > >>> the > >> > > > >>> >> last > >> > > > >>> >> >> consumed offset? I think I'm with Colin in preferring > >> > > > >>> >> >> RecordDeserializationException for both cases if > >> possible. For > >> > > > one > >> > > > >>> >> thing, > >> > > > >>> >> >> that makes the behavior consistent whether or not > >> `check.crs` > >> > > is > >> > > > >>> >> enabled. > >> > > > >>> >> >> > >> > > > >>> >> >> -Jason > >> > > > >>> >> >> > >> > > > >>> >> >> On Tue, Aug 7, 2018 at 11:17 AM, Colin McCabe < > >> > > > cmcc...@apache.org> > >> > > > >>> >> wrote: > >> > > > >>> >> >> > >> > > > >>> >> >>> Hi Stanislav, > >> > > > >>> >> >>> > >> > > > >>> >> >>> On Sat, Aug 4, 2018, at 10:44, Stanislav Kozlovski wrote: > >> > > > >>> >> >>>> Hey Colin, > >> > > > >>> >> >>>> > >> > > > >>> >> >>>> It may be a bit vague but keep in mind we also raise the > >> > > > >>> exception in > >> > > > >>> >> >> the > >> > > > >>> >> >>>> case where the record is corrupted. > >> > > > >>> >> >>>> We discussed with Jason offline that message corruption > >> most > >> > > > >>> often > >> > > > >>> >> >>> prevents > >> > > > >>> >> >>>> deserialization itself and that may be enough of an > >> argument > >> > > to > >> > > > >>> raise > >> > > > >>> >> >>>> `RecordDeserializationException` in the case of a > >> corrupt > >> > > > >>> record. I > >> > > > >>> >> >>>> personally find that misleading. > >> > > > >>> >> >>> > >> > > > >>> >> >>> Hmm. I think that by definition, corrupt records are > >> records > >> > > > that > >> > > > >>> >> can't > >> > > > >>> >> >>> be deserialized. There's no difference between the two > >> cases > >> > > -- > >> > > > >>> if > >> > > > >>> >> (and > >> > > > >>> >> >>> only if) the message is corrupt, it can't be > >> deserialized. If > >> > > > >>> (and > >> > > > >>> >> only > >> > > > >>> >> >>> if) it can't be deserialized, it is corrupt. > >> > > > >>> >> >>> > >> > > > >>> >> >>>> > >> > > > >>> >> >>>> In the end, I think it might be worth it to have a bit > >> of a > >> > > > >>> >> >>>> wider-encompassing `FaultyRecordException` (or even > >> > > > >>> >> >>>> `UnconsumableRecordException`) which would allow users > >> to > >> > > skip > >> > > > >>> over > >> > > > >>> >> the > >> > > > >>> >> >>>> record. > >> > > > >>> >> >>> > >> > > > >>> >> >>> If you are inheriting from SerializationException, your > >> > > derived > >> > > > >>> class > >> > > > >>> >> >>> should also be a kind of serialization exception. Not > >> > > something > >> > > > >>> more > >> > > > >>> >> >>> general. > >> > > > >>> >> >>> > >> > > > >>> >> >>>> We could then potentially have more specific exceptions > >> (e.g > >> > > > >>> >> >>>> deserialization) inherit that but I'm not sure if we > >> should. > >> > > > >>> >> >>>> A case for a more general exception which provides > >> access to > >> > > > the > >> > > > >>> >> >>>> partition/offset is future backwards-compatibility. If > >> there > >> > > is > >> > > > >>> ever > >> > > > >>> >> a > >> > > > >>> >> >>> new > >> > > > >>> >> >>>> scenario that would make the user need to skip a > >> specific > >> > > > record > >> > > > >>> - > >> > > > >>> >> >> there > >> > > > >>> >> >>>> would already be such an exception and this will > >> provide some > >> > > > >>> >> >>>> backward-compatibility with older clients. > >> > > > >>> >> >>> > >> > > > >>> >> >>> Hmm. Can you think of any new scenarios that would make > >> Kafka > >> > > > >>> force > >> > > > >>> >> the > >> > > > >>> >> >>> user need to skip a specific record? Perhaps one > >> scenario is > >> > > if > >> > > > >>> >> records > >> > > > >>> >> >>> are lost but we don't know how many. > >> > > > >>> >> >>> > >> > > > >>> >> >>> If we really want to have something more general, > >> perhaps we > >> > > > need > >> > > > >>> >> >>> something like LostDataException. But in that case, we > >> can't > >> > > > >>> inherit > >> > > > >>> >> >> from > >> > > > >>> >> >>> SerializationException, since lost data is more general > >> than > >> > > > >>> >> >> serialization > >> > > > >>> >> >>> issues. > >> > > > >>> >> >>> > >> > > > >>> >> >>> best, > >> > > > >>> >> >>> Colin > >> > > > >>> >> >>> > >> > > > >>> >> >>> > >> > > > >>> >> >>>> > >> > > > >>> >> >>>> Best, > >> > > > >>> >> >>>> Stanislav > >> > > > >>> >> >>>> > >> > > > >>> >> >>>> On Sat, Aug 4, 2018 at 12:23 AM Colin McCabe < > >> > > > cmcc...@apache.org > >> > > > >>> > > >> > > > >>> >> >> wrote: > >> > > > >>> >> >>>> > >> > > > >>> >> >>>>> Hi Stanislav, > >> > > > >>> >> >>>>> > >> > > > >>> >> >>>>> Thanks for the KIP. > >> > > > >>> >> >>>>> > >> > > > >>> >> >>>>> As as user, "FaultyRecordException" seems a little > >> vague. > >> > > > >>> What's > >> > > > >>> >> >>> faulty > >> > > > >>> >> >>>>> about it? Is it too big? Does it not match the > >> schema of > >> > > the > >> > > > >>> other > >> > > > >>> >> >>>>> records? Was it not found? > >> > > > >>> >> >>>>> > >> > > > >>> >> >>>>> Of course, we know that the specific problem is with > >> > > > >>> [de]serializing > >> > > > >>> >> >>> the > >> > > > >>> >> >>>>> record, not with any of those those things. So we > >> should > >> > > > >>> choose a > >> > > > >>> >> >> name > >> > > > >>> >> >>>>> that reflects that serialization is the problem. How > >> about > >> > > > >>> >> >>>>> RecordSerializationException? > >> > > > >>> >> >>>>> > >> > > > >>> >> >>>>> best, > >> > > > >>> >> >>>>> Colin > >> > > > >>> >> >>>>> > >> > > > >>> >> >>>>> > >> > > > >>> >> >>>>> On Thu, Aug 2, 2018, at 15:11, Stanislav Kozlovski > >> wrote: > >> > > > >>> >> >>>>>> Hi Jason and Ted, > >> > > > >>> >> >>>>>> > >> > > > >>> >> >>>>>> @Jason > >> > > > >>> >> >>>>>> I did not oppose the idea as I'm not too familiar > >> with Java > >> > > > >>> >> >>> conventions. > >> > > > >>> >> >>>>> I > >> > > > >>> >> >>>>>> agree it is a non-ideal way for the user to catch the > >> > > > >>> exception so > >> > > > >>> >> >> I > >> > > > >>> >> >>>>>> changed it back. > >> > > > >>> >> >>>>>> > >> > > > >>> >> >>>>>> I've updated the KIP and PR with the latest changes. > >> Now, > >> > > > >>> there is > >> > > > >>> >> >>> only > >> > > > >>> >> >>>>> one > >> > > > >>> >> >>>>>> public exception `FaultyRecordException` which is > >> thrown to > >> > > > the > >> > > > >>> >> >> user > >> > > > >>> >> >>> in > >> > > > >>> >> >>>>>> both scenarios (corrupt record and deserialization > >> error). > >> > > > >>> >> >>>>>> Please take a look again. > >> > > > >>> >> >>>>>> > >> > > > >>> >> >>>>>> Best, > >> > > > >>> >> >>>>>> Stanislav > >> > > > >>> >> >>>>>> > >> > > > >>> >> >>>>>> On Thu, Aug 2, 2018 at 5:25 PM Jason Gustafson < > >> > > > >>> ja...@confluent.io > >> > > > >>> >> >>> > >> > > > >>> >> >>>>> wrote: > >> > > > >>> >> >>>>>> > >> > > > >>> >> >>>>>>> Hey Stanislav, > >> > > > >>> >> >>>>>>> > >> > > > >>> >> >>>>>>> I should have noticed it earlier from your example, > >> but I > >> > > > just > >> > > > >>> >> >>> realized > >> > > > >>> >> >>>>>>> that interfaces don't mix well with exceptions. > >> There is > >> > > no > >> > > > >>> way > >> > > > >>> >> >> to > >> > > > >>> >> >>>>> catch > >> > > > >>> >> >>>>>>> the interface type, which means you have to depend on > >> > > > >>> instanceof > >> > > > >>> >> >>>>> checks, > >> > > > >>> >> >>>>>>> which is not very conventional. At the moment, we > >> raise > >> > > > >>> >> >>>>>>> SerializationException if there is a problem parsing > >> the > >> > > > >>> error, > >> > > > >>> >> >>> and we > >> > > > >>> >> >>>>>>> raise KafkaException if the record fails its CRC > >> check. > >> > > > Since > >> > > > >>> >> >>>>>>> SerializationException extends KafkaExeption, it > >> seems > >> > > like > >> > > > we > >> > > > >>> >> >> can > >> > > > >>> >> >>>>> handle > >> > > > >>> >> >>>>>>> both cases in a compatible way by handling both cases > >> > > with a > >> > > > >>> >> >> single > >> > > > >>> >> >>>>> type > >> > > > >>> >> >>>>>>> that extends SerializationException. Maybe something > >> like > >> > > > >>> >> >>>>>>> RecordDeserializationException? > >> > > > >>> >> >>>>>>> > >> > > > >>> >> >>>>>>> Thanks, > >> > > > >>> >> >>>>>>> Jason > >> > > > >>> >> >>>>>>> > >> > > > >>> >> >>>>>>> On Thu, Aug 2, 2018 at 5:45 AM, Ted Yu < > >> > > yuzhih...@gmail.com > >> > > > > > >> > > > >>> >> >>> wrote: > >> > > > >>> >> >>>>>>> > >> > > > >>> >> >>>>>>>> +1 > >> > > > >>> >> >>>>>>>> -------- Original message --------From: Stanislav > >> > > > Kozlovski < > >> > > > >>> >> >>>>>>>> stanis...@confluent.io> Date: 8/2/18 2:41 AM > >> > > (GMT-08:00) > >> > > > >>> To: > >> > > > >>> >> >>>>>>>> dev@kafka.apache.org Subject: [VOTE] KIP-334 > >> Include > >> > > > >>> >> >> partitions > >> > > > >>> >> >>> in > >> > > > >>> >> >>>>>>>> exceptions raised during consumer record > >> > > > >>> >> >>> deserialization/validation > >> > > > >>> >> >>>>>>>> Hey everybody, > >> > > > >>> >> >>>>>>>> > >> > > > >>> >> >>>>>>>> I'd like to start a vote thread for KIP-334 Include > >> > > > >>> partitions > >> > > > >>> >> >> in > >> > > > >>> >> >>>>>>>> exceptions raised during consumer record > >> > > > >>> >> >>> deserialization/validation > >> > > > >>> >> >>>>>>>> < > >> > > > >>> >> >>>>>>> > >> > > > >>> >> >>>>> > >> https://cwiki.apache.org/confluence/pages/viewpage.action? > >> > > > >>> >> >>> pageId=87297793 > >> > > > >>> >> >>>>>>>>> > >> > > > >>> >> >>>>>>>> > >> > > > >>> >> >>>>>>>> -- > >> > > > >>> >> >>>>>>>> Best, > >> > > > >>> >> >>>>>>>> Stanislav > >> > > > >>> >> >>>>>>>> > >> > > > >>> >> >>>>>>> > >> > > > >>> >> >>>>>> > >> > > > >>> >> >>>>>> > >> > > > >>> >> >>>>>> -- > >> > > > >>> >> >>>>>> Best, > >> > > > >>> >> >>>>>> Stanislav > >> > > > >>> >> >>>>> > >> > > > >>> >> >>>> > >> > > > >>> >> >>>> > >> > > > >>> >> >>>> -- > >> > > > >>> >> >>>> Best, > >> > > > >>> >> >>>> Stanislav > >> > > > >>> >> >>> > >> > > > >>> >> >> > >> > > > >>> >> > > >> > > > >>> >> > > >> > > > >>> >> > >> > > > >>> >> > >> > > > >>> > >> > > > >> > >> > > > >> > >> > > > >> -- > >> > > > >> Best, > >> > > > >> Stanislav > >> > > > >> > >> > > > > > >> > > > > > >> > > > > -- > >> > > > > Best, > >> > > > > Stanislav > >> > > > > > >> > > > > >> > > > > >> > > > -- > >> > > > Best, > >> > > > Stanislav > >> > > > > >> > > > >> > > >> > > >> > -- > >> > Best, > >> > Stanislav > >> > > > > > > -- > > Best, > > Stanislav > > > > > -- > > <https://www.confluent.io/> > > *Sarwar Bhuiyan* > > Senior Customer Success Architect > > +447949684437 > > Follow us: Blog <http://www.confluent.io/blog> • Slack > <https://slackpass.io/confluentcommunity> • Twitter > <https://twitter.com/ConfluentInc> • YouTube <https://youtube.com/confluent> > > <https://developer.confluent.io/> >
