Hi Sophie,
I have updated the KIP and the PR.
Regards,
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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`?
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
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
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
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
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
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
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
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
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
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.
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
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
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
29 matches
Mail list logo