Re: [DISCUSS] KIP-1036: Extend RecordDeserializationException exception

Hi Sophie, I have updated the KIP and the PR. Regards,

Re: [DISCUSS] KIP-1036: Extend RecordDeserializationException exception

Sounds good -- exceptionOrigin makes sense to me. Let us know when you've updated the KIP. I'll cast a vote once these last small changes have been made On Thu, May 2, 2024 at 10:16 AM Frédérik Rouleau wrote: > Hi Sophie, > > I agree that the subclasses have limited value and I am not a fan of

Re: [DISCUSS] KIP-1036: Extend RecordDeserializationException exception

Hi Sophie, I agree that the subclasses have limited value and I am not a fan of "instance of" usage either. I do not see any problem with adding a field but I would rather name it something like exceptionOrigin. Any thoughts? About byteBuffer vs byte[], byteBuffer are more generic and with

Re: [DISCUSS] KIP-1036: Extend RecordDeserializationException exception

Actually one more thing, after thinking a bit about how this would be used in practice, I'm inclined to agree that maybe distinguishing between key vs value errors via subclasses is not the cleanest way to present the API. Users would still essentially want to catch the general

Re: [DISCUSS] KIP-1036: Extend RecordDeserializationException exception

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

Re: [DISCUSS] KIP-1036: Extend RecordDeserializationException exception

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

Re: [DISCUSS] KIP-1036: Extend RecordDeserializationException exception

Hi Fred, Thanks for the updates! Questions: K11. Can we reconsider the introduction of two new exception subclasses? Perhaps I don’t understand the benefit? Technically both the key and the value could have deserialization errors, right? K12. Is there a benefit to exposing both the

Re: [DISCUSS] KIP-1036: Extend RecordDeserializationException exception

Hi Andrew, A1. I will change the order of arguments to match. A2 and A3, Yes the KIP is not updated yet as I do not have a wiki account. So I must rely on some help to do those changes, which add some delay. I will try to find someone available ASAP. A4. I had the same thought. Using keyBuffer

Re: [DISCUSS] KIP-1036: Extend RecordDeserializationException exception

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

Re: [DISCUSS] KIP-1036: Extend RecordDeserializationException exception

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

Re: [DISCUSS] KIP-1036: Extend RecordDeserializationException exception

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

Re: [DISCUSS] KIP-1036: Extend RecordDeserializationException exception

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

Re: [DISCUSS] KIP-1036: Extend RecordDeserializationException exception

Andrew, thanks for the details about Consumer internals. That's super useful for this discussion! -- And it confirms my understanding. I don't think we want to use ConsumerRecord type thought, because for a DLQ the handler wants to write the message into some DLQ topic, and thus needs the key

Re: [DISCUSS] KIP-1036: Extend RecordDeserializationException exception

Hi, Thanks for the KIP. I think it’s an interesting idea and it seems to work nicely with how the clients work today. Recently, I’ve been digging in to the record deserialization code in the consumer as part of implementing KIP-932. It’s pretty nasty in there. There are essentially two kinds

Re: [DISCUSS] KIP-1036: Extend RecordDeserializationException exception

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

Re: [DISCUSS] KIP-1036: Extend RecordDeserializationException exception

Hi, I am actually not sure if using `Record` is really the right thing? While `Record` is technically public API, it does not seem to be intended to be exposed to end users? But I guess my main question is really about what metadata we really want to add to `RecordDeserializationException`?

Re: [DISCUSS] KIP-1036: Extend RecordDeserializationException exception

Also ignore everything I said about Streams earlier. I didn't look closely enough on my first pass over the KIP and thought this was changing the DeserializationExceptionHandler in Streams. I see now that this is actually about the consumer client's DeserializationException so everything I said

Re: [DISCUSS] KIP-1036: Extend RecordDeserializationException exception

Ah, thanks for the additional context. I should have looked at the code before I opened my mouth (so to speak) In that case, I fully agree that using Record instead of ConsumerRecord makes sense. It does indeed seem like by definition, if there is a DeserializationException then there is no

Re: [DISCUSS] KIP-1036: Extend RecordDeserializationException exception

Thanks Sophie, I can write something in the KIP on how KStreams solves that issue, but as I can't create a Wiki account, I will have to find someone to do this on my behalf (if someone can work on solving that wiki account creation, it would be great). The biggest difference between Record and

Re: [DISCUSS] KIP-1036: Extend RecordDeserializationException exception

As for the ConsumerRecord vs Record thing -- I personally think the other alternative that Kirk mentioned would make more sense here, that is, returning a Optional> rather than changing the type from ConsumerRecord to Record. I'm not sure why checkstyle is saying we shouldn't use the Record

Re: [DISCUSS] KIP-1036: Extend RecordDeserializationException exception

I think some missing context here (which can maybe be added in the Motivation section as background) is that the deserialization is actually done within Streams, not within the Consumer. Since the consumers in Kafka Streams might be subscribed to multiple topics with different data types, it has

Re: [DISCUSS] KIP-1036: Extend RecordDeserializationException exception

Hi Almog, I think you do not understand the behavior that was introduced with the KIP-334. When you have a DeserializationException, if you set the proper seek call to skip the faulty record, the next poll call will return the remaining records to process and not a new list of records. When the

Re: [DISCUSS] KIP-1036: Extend RecordDeserializationException exception

Hi Frederik - thanks for the KIP, this will be a fantastic and elegant addition to Kafka Streams. I have a higher level question about this, which is that the `poll` interface returns multiple records and yet the DeserializationException will be thrown if any record in the batch cannot be

Re: [DISCUSS] KIP-1036: Extend RecordDeserializationException exception

I think the bigger question here is: why is checkstyle complaining about this import? Does anyone know? On Thu, Apr 11, 2024 at 11:12 AM Frédérik Rouleau wrote: > Hi everyone, > > I have made some changes to take in account comments. I have replaced the > ConsumerRecord by Record. As it was not

Re: [DISCUSS] KIP-1036: Extend RecordDeserializationException exception

Hi everyone, I have made some changes to take in account comments. I have replaced the ConsumerRecord by Record. As it was not allowed by checkstyle, I have modified its configuration. I hope that's ok. I find this new version better. Thanks for your help. Regards, Fred

Re: [DISCUSS] KIP-1036: Extend RecordDeserializationException exception

Hi Kirk, I have made the test and I confirm that checkstyle is complaining (Disallowed import - org.apache.kafka.common.record.Record.) if I use Record class in the RecordDeserialisationException. An alternative might be to add key(), value(), headers() etc methods directly in the exception.

Re: [DISCUSS] KIP-1036: Extend RecordDeserializationException exception

Thanks for your feedback. Kirk, that's a good point. I will check if there are other ways of raising an exception than the deserialisation itself. About Record, I agree I think it would be a better choice and my initial version was using it. But then I realised that this class might not be

Re: [DISCUSS] KIP-1036: Extend RecordDeserializationException exception

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

Re: [DISCUSS] KIP-1036: Extend RecordDeserializationException exception

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

[DISCUSS] KIP-1036: Extend RecordDeserializationException exception

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 . Thanks for