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
