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