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 > >> > > >
