Thanks for all the context everyone. I think in light of all the information that's been shared, I would agree with David that neither the Record nor ConsumerRecord feels appropriate and that we should just create a new class/interface that holds the info we want to expose. We definitely don't want to add each item as a separate parameter since that will make evolution of this API difficult to do without being a hassle for users (ie migrating off deprecated APIs and/or blowing up the API surface area).
I also like the idea of using this to indicate what/where the actual exception occurred. Alternatively, we could literally extend the RecordDeserializationException exception class and have a separate exception type to indicate whether the failure occurred when deserializing the key or value. Not necessarily saying this is better than just adding info to the container class/interface, just listing all the options. My impression is that we tend to favor presenting info in exceptions by extending the exception type itself rather than adding data to the exception class. I'm not sure I even agree with that pattern, but that's been my observation so far. On Thu, Apr 18, 2024 at 10:47 AM David Arthur <david.art...@confluent.io.invalid> wrote: > Hi Fred, thanks for the KIP. Seems like a useful improvement. > > As others have mentioned, I think we should avoid exposing Record in this > way. > > Using ConsumerRecord seems okay, but maybe not the best fit for this case > (for the reasons Matthias gave). > > Maybe we could create a new container interface to hold the partially > deserialized data? This could also indicate to the exception handler > whether the key, the value, or both had deserialization errors. > > Thanks, > David > > On Thu, Apr 18, 2024 at 10:16 AM Frédérik Rouleau > <froul...@confluent.io.invalid> wrote: > > > Hi, > > > > But I guess my main question is really about what metadata we really > > > want to add to `RecordDeserializationException`? `Record` expose all > > > kind of internal (serialization) metadata like `keySize()`, > > > `valueSize()` and many more. For the DLQ use-case it seems we don't > > > really want any of these? So I am wondering if just adding > > > key/value/ts/headers would be sufficient? > > > > > > > I think that key/value/ts/headers, topicPartition and offset are all we > > need. I do not see any usage for other metadata. If someone has a use > case, > > I would like to know it. > > > > So in that case we can directly add the data into the exception. We can > > keep ByteBuffer for the local field instead of byte[], that will avoid > > memory allocation if users do not require it. > > I wonder if we should return the ByteBuffer or directly the byte[] (or > both > > ?) which is more convenient for end users. Any thoughts? > > Then we can have something like: > > > > public RecordDeserializationException(TopicPartition partition, > > long offset, > > ByteBuffer key, > > ByteBuffer value, > > Header[] headers, > > long timestamp, > > String message, > > Throwable cause); > > > > public TopicPartition topicPartition(); > > > > public long offset(); > > > > public long timestamp(); > > > > public byte[] key(); // Will allocate the array on call > > > > public byte[] value(); // Will allocate the array on call > > > > public Header[] headers(); > > > > > > > > Regards, > > Fred > > > > > -- > -David >
