Sounds good -- exceptionOrigin makes sense to me. Let us know when you've updated the KIP. I'll cast a vote once these last small changes have been made
On Thu, May 2, 2024 at 10:16 AM Frédérik Rouleau <froul...@confluent.io.invalid> wrote: > Hi Sophie, > > I agree that the subclasses have limited value and I am not a fan of > "instance of" usage either. > I do not see any problem with adding a field but I would rather name it > something like exceptionOrigin. Any thoughts? > > About byteBuffer vs byte[], byteBuffer are more generic and with proper > doc/example, I do not think it's an issue. I will then remove the byte[] > returning methods. > > Thanks, > > > [image: Confluent] <https://www.confluent.io> > Frederik Rouleau > Sr Customer Success Technical Architect > Follow us: [image: Blog] > < > https://www.confluent.io/blog?utm_source=footer&utm_medium=email&utm_campaign=ch.email-signature_type.community_content.blog > >[image: > Twitter] <https://twitter.com/ConfluentInc>[image: LinkedIn] > <https://www.linkedin.com/company/confluent/>[image: Slack] > <https://slackpass.io/confluentcommunity>[image: YouTube] > <https://youtube.com/confluent> > > [image: Try Confluent Cloud for Free] > < > https://www.confluent.io/get-started?utm_campaign=tm.fm-apac_cd.inbound&utm_source=gmail&utm_medium=organic > > > > > On Tue, Apr 30, 2024 at 10:54 PM Sophie Blee-Goldman < > sop...@responsive.dev> > wrote: > > > Actually one more thing, after thinking a bit about how this would be > used > > in practice, I'm inclined to agree that maybe distinguishing between key > vs > > value errors via subclasses is not the cleanest way to present the API. > > Users would still essentially want to catch the general > > RecordDeserializationException error since in practice, much of the > > handling is likely to be the same between key and value errors. So then > to > > distinguish between these, they would end up doing an "instance of" check > > on the exception type. Which feels like an awkward way to answer a > question > > that could have just been a simple API on the > > RecordDeserializationException itself. What do you think about getting > rid > > of the subclasses and instead adding one more API to the > > RecordDeserializationException that indicates whether it was a key or > value > > error? > > > > This could return a simple boolean and be called #isKeyError or > something, > > but that feels kind of awkward too. Maybe a better alternative would be > > something like this: > > > > class RecordDeserializationException { > > enum DeserializationExceptionType { > > KEY, > > VALUE > > } > > > > public DeserializationExceptionType exceptionType(); > > } > > > > I also worry that people don't always check for exception subtypes and > > would easily miss the existence of the KeyDeserializationException and > > ValueDeserializationException. Simply adding an API to the > > RecordDeserializationException will make it much easier for users to > notice > > and react accordingly, if they care to do something different based on > > whether the error happened during key or value deserialization. > > > > Thoughts? > > > > On Tue, Apr 30, 2024 at 1:45 PM Sophie Blee-Goldman < > sop...@responsive.dev > > > > > wrote: > > > > > Hey Fred, I think this is looking good but I just want to follow up on > > > what Kirk asked earlier about having both the ByteBuffer and byte[] > > forms. > > > Can't users just use the ByteBuffer versions and convert them into a > > byte[] > > > themselves? In some cases it maybe makes sense to offer some additional > > > APIs if there is complex processing involved in converting between > > returned > > > types, but ByteBuffer --> byte[] seems pretty straightforward to me :) > > > > > > Generally speaking we try to keep the APIs as tight as possible and > offer > > > only what is necessary, and I'd rather leave off "syntactic sugar" type > > > APIs unless there is a clear need. Put another way: it's easy to add > > > additional methods if someone wants them, but it's much harder to > remote > > > methods since we have to go through a deprecation cycle. So I'd prefer > to > > > just keep only the ByteBuffer versions (or only the byte[] -- don't > > > personally care which of the two) > > > > > > One more small nit: since we're deprecating the old exception > > constructor, > > > can you list that in the "Compatibility, Deprecation, and Migration > Plan" > > > section? > > > > > > > > > > > > On Wed, Apr 24, 2024 at 5:35 AM Frédérik Rouleau > > > <froul...@confluent.io.invalid> wrote: > > > > > >> Hi, > > >> > > >> I have updated the KIP now and the latest version of PR is available. > > >> > > >> About Kirk's questions: > > >> > > >> K11: Yes, both can have a deserialization exception but we deserialize > > the > > >> key first, so if an error occurs then, we do not try to deserialize > the > > >> value. So the exception raised is always for key or value. > > >> > > >> K12: Not sure of concrete usage for now, just a sugar feature. I > suppose > > >> we > > >> can imagine some use case where you need/want only the first bytes and > > do > > >> not want to waste memory allocating the whole payload > (SchemaRegistry's > > >> schema Id or something similar). > > >> > > >> K13: The old constructor is not needed anymore. It is just for > > >> compatibility until removed in a major version. As public we might > have > > >> some users using it even if I cannot see any valid reason for this. > > >> > > >> Thanks, > > >> Fred > > >> > > > > > >
