Hi Fred, Just reviewing the KIP again now that the discussion has quietened down a little. It will soon be ready for a vote I think. I have a few comments about details.
A1) The signature of the new constructor for RecordDeserializationException needs to be updated accord to the discussion. I see that you have a PR which is much closer to what I expected. Personally, I think the arguments for the constructor which represent the portions of the record should match the order for the constructor of ConsumerRecord. We’ve already worked out the order of these things once so I would go for consistency. I suggest: public RecordDeserializationException( TopicPartition partition, long offset, long timestamp, TimestampType timestampType, ByteBuffer key, ByteBuffer value, Headers headers, String message, Throwable cause); A2) There are still references to the Record class in the KIP, but we decided not to use it. A3) There is also a reference to a getConsumerRecord() method which is now to be replaced by individual methods for the portions of the record, such as: byte[] value(); The KIP should have a complete and accurate description of the Java interface changes so please fill in the details. A4) Given that the key and value are provided to the constructor as ByteBuffer but lazily converted into byte[] as required, I wonder whether the names of the methods and the constructor arguments should be slightly different, just a keyBuffer for the constructure and key() for the getter. Maybe you prefer to keep them the same and I’m happy with that. Just offering a suggestion. Thanks for the KIP. I think it’s a worthwhile improvement and I expect it’s almost there. Thanks, Andrew > On 19 Apr 2024, at 18:59, Frédérik Rouleau <froul...@confluent.io.INVALID> > wrote: > > Hi everyone, > > Thanks for all that valuable feedback. > So we have a consensus not to use Record. > > I have updated to PR by creating 2 childs classes > KeyDeserializationException and ValueDeserializationException. Those > classes directly embed the required fields. I do not think a wrapper object > would be useful in that case. > I still had to update checkstyle as Headers class is not allowed for import > in the Errors package. I do not think it's an issue to add that > authorization as Headers is already used in consumerRecord, so already > public. > > The proposed PR https://github.com/apache/kafka/pull/15691/files > > If it's ok I will update the KIP. > > Regards, > Fred
