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 >
