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
> 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
>>>
>>
