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
>
