Hi,
the KIP looks great!
public static final String PROCESS_EXCEPTION_HANDLER_CLASS_CONFIG =
"process.exception.handler".
needs to be changed to
public static final String PROCESSING_EXCEPTION_HANDLER_CLASS_CONFIG =
"processing.exception.handler".
The name of the constant has been already corrected in the code block
but the actual name of the config (i.e., the content of the constant)
has not been changed yet.
Best,
Bruno
On 5/3/24 10:35 AM, Sebastien Viale wrote:
Hi,
So, we all agree to revert to the regular Headers interface in
ErrorHandlerContext.
We will update the KIP accordingly.
@Sophie => Yes, this is the last remaining question, and it has been open for
voting since last week.
Thanks
Sébastien
________________________________
De : Andrew Schofield <andrew_schofi...@live.com>
Envoyé : vendredi 3 mai 2024 06:44
À : dev@kafka.apache.org <dev@kafka.apache.org>
Objet : [SUSPICIOUS EXTERNAL MESSAGE] Re: [DISCUSS] KIP-1033: Add Kafka Streams
exception handler for exceptions occuring during processing
Warning This might be a fraudulent message! When clicking REPLY, your answers
will NOT go to the sender (andrew_schofi...@live.com). Instead, replies will be
sent to dev@kafka.apache.org. Be cautious!
Hi,
I’ve changed my mind on this one having read through the comments.
I don’t think the exception handler should be able to mess with the headers
to the detriment of the code that called the handler.
While I like the hygiene of having an ImmutableHeaders interface,
I feel we can use the existing interface to get the effect we desire.
Thanks,
Andrew
========================================================================================
This email was screened for spam and malicious content but exercise caution
anyway.
On 3 May 2024, at 03:40, Sophie Blee-Goldman <sop...@responsive.dev> wrote:
I tend to agree that we should just return a pure Headers instead of
introducing a new class/interface to protect overwriting them. I think a
pretty good case has been made already so I won't add onto it, just wanted
to voice my support.
Is that the only remaining question on this KIP? Might be ok to move to a
vote now?
On Wed, May 1, 2024 at 8:05 AM Lianet M. <liane...@gmail.com> wrote:
Hi all, thanks Damien for the KIP!
After looking into the KIP and comments, my only concern is aligned with
one of Matthias comments, around the ImmutableHeaders introduction, with
the motivation not being clear enough. The existing handlers already expose
the headers (indirectly). Ex.
ProductionExceptionHandler.handleSerializationException provides the
ProducerRecord as an argument, so they are already exposed in those
callbacks through record.headers(). Is there a reason to think that it
would be a problem to expose the headers in the
new ProcessingExceptionHandler, but that it's not a problem for the
existing handler?
If there is no real concern about the KS engine requiring those headers, it
feels hard to mentally justify the complexity we transfer to the user by
exposing a new concept into the callbacks to represent the headers. In the
end, it strays aways from the simple/consistent representation of Headers
used all over. Even if eventually the KS engine needs to use the headers
after the callbacks with certainty that they were not altered, still feels
like it's something we could attempt to solve internally, without having to
transfer "new concepts" into the user (ex. the deep-copy as it was
suggested, seems like the kind of trade-off that would maybe be acceptable
here to gain simplicity and consistency among the handlers with a single
existing representation of Headers).
Best!
Lianet
On Tue, Apr 30, 2024 at 9:36 PM Matthias J. Sax <mj...@apache.org> wrote:
Thanks for the update.
I am wondering if we should use `ReadOnlyHeaders` instead of
`ImmutableHeaders` as interface name?
Also, the returned `Header` interface is technically not immutable
either, because `Header#key()` returns a mutable byte-array... Would we
need a `ReadOnlyHeader` interface?
If yes, it seems that `ReadOnlyHeaders` should not be a super-interface
of `Headers` but it would rather be a standalone interface, and a
wrapper for a `Headers` instance? And `ReadOnlyHeader` would return some
immutable type instead of `byte[]` for the value()?
An alternative would be to deep-copy the value byte-array what would not
be free, but given that we are talking about exception handling, it
would not be on the hot code path, and thus might be acceptable?
The above seems to increase the complexity significantly though. Hence,
I have seconds thoughts on the immutability question:
Do we really need to worry about mutability after all, because in the
end, KS runtime won't read the Headers instance after the handler was
called, and if a user modifies the passed in headers, there won't be any
actual damage (ie, no side effects)? For this case, it might even be ok
to also not add `ImmutableHeaders` to begin with?
Sorry for the forth and back (yes, forth and back, because back and
forth does not make sense -- it's not logical -- just trying to fix
English :D) as I did bring up the immutability question in the first
place...
-Matthias
On 4/25/24 5:56 AM, Loic Greffier wrote:
Hi Matthias,
I have updated the KIP regarding points 103 and 108.
103.
I have suggested a new `ImmutableHeaders` interface to deal with the
immutability concern of the headers, which is basically the `Headers`
interface without the write accesses.
public interface ImmutableHeaders {
Header lastHeader(String key);
Iterable<Header> headers(String key);
Header[] toArray();
}
The `Headers` interface can be updated accordingly:
public interface Headers extends ImmutableHeaders, Iterable<Header> {
//…
}
Loïc
