Thanks for the KIP Fred.
Couple of nits: it's not clear from the "Public API" section what is new
and what is existing API w/o going back to the code. For existing
methods which are not changed, it's also best to actually omit them. --
It would also be best to only put the interface itself down, but not the
implementation (ie, no private members and no method body).
Thus, it might be better to do something like this:
+++++
public class RecordDeserializationException extends SerializationException {
// newly added
public RecordDeserializationException(TopicPartition partition,
ConsumerRecord<byte[], byte[]>
record,
String message,
Throwable cause);
public ConsumerRecord<byte[], byte[]> getConsumerRecord();
}
+++++
From the description it's not clear to me if you propose to change the
existing constructor, or propose to add a new constructor. From a
compatibility POV, we cannot really change the existing constructor (but
we could deprecate it and remove in the future (and add a new one in
parallel). But I also agree with Kirk that there could be cases for
which we cannot pass in a `ConsumerRecord` and thus keeping the old
constructor could make sense (and change the new getter to return an
`Optinal`).
Another small thing: in Kafka, getter methods are not using a `get`
prefix, and thus it should be `consumerRecord()` w/o the "get".
-Matthias
On 4/10/24 4:21 PM, Kirk True wrote:
Hi Fred,
Thanks for the KIP!
Questions/comments:
How do we handle the case where CompletedFetch.parseRecord isn’t able to
construct a well-formed ConsumerRecord (i.e. the values it needs are
missing/corrupted/etc.)?
Please change RecordDeserializationException’s getConsumerRecord() method to be
named consumerRecord() to be consistent.
Should we change the return type of consumerRecord() to be
Optional<ConsumerRecord<byte[], byte[]>> in the cases where even a “raw”
ConsumerRecord can’t be created?
To avoid the above, does it make sense to include a Record object instead of a
ConsumerRecord? The former doesn’t include the leaderEpoch or TimestampType,
but maybe that’s OK?
Thanks,
Kirk
On Apr 10, 2024, at 8:47 AM, Frédérik Rouleau <froul...@confluent.io.INVALID>
wrote:
Hi everyone,
To make implementation of DLQ in consumer easier, I would like to add the
raw ConsumerRecord into the RecordDeserializationException.
Details are in KIP-1036
<https://cwiki.apache.org/confluence/display/KAFKA/KIP-1036%3A+Extend+RecordDeserializationException+exception>
.
Thanks for your feedback.
Regards,
Fred
