kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthias J. Sax" <matth...@confluent.io>
Subject Re: [DISCUSS] KIP-210: Provide for custom error handling when Kafka Streams fails to produce
Date Wed, 13 Dec 2017 23:45:12 GMT
One more after thought: should we add a metric for this? We also have a
metric for `skippedDueToDeserializationError-rate` ?


-Matthias



On 12/6/17 7:54 AM, Bill Bejeck wrote:
> Thanks for the clearly written KIP, no further comments from my end.
> 
> -Bill
> 
> On Wed, Dec 6, 2017 at 9:52 AM, Matt Farmer <matt@frmr.me> wrote:
> 
>> There is already a vote thread for this KIP. I can bump it so that it’s
>> towards the top of your inbox.
>>
>> With regard to your concerns:
>>
>> 1) We do not have the "ProductionExceptionHandler" interface defined in the
>> wiki page, thought it is sort of clear that it is a one-function interface
>> with record and exception. Could you add it?
>>
>>
>> It is defined, it’s just not defined using a code snippet. The KIP reads as
>> follows:
>>
>> ===
>>
>> A public interface named ProductionExceptionHandler with a single method,
>> handle, that has the following signature:
>>
>>    - ProductionExceptionHandlerResponse handle(ProducerRecord<byte[],
>>    byte[]> record, Exception exception)
>>
>>
>> ===
>>
>> If you’d like me to add a code snippet illustrating this that’s simple for
>> me to do, but it seemed superfluous.
>>
>> 2) A quick question about your example code: where would be the "logger"
>> object be created?
>>
>>
>> SLF4J loggers are typically created as a class member in the class. Such
>> as:
>>
>> private Logger logger = LoggerFactory.getLogger(HelloWorld.class);
>>
>> I omit that in my implementation examples for brevity.
>>
>> On December 6, 2017 at 2:14:58 AM, Guozhang Wang (wangguoz@gmail.com)
>> wrote:
>>
>> Hello Matt,
>>
>> Thanks for writing up the KIP. I made a pass over it and here is a few
>> minor comments. I think you can consider starting a voting thread for this
>> KIP while addressing them.
>>
>> 1) We do not have the "ProductionExceptionHandler" interface defined in the
>> wiki page, thought it is sort of clear that it is a one-function interface
>> with record and exception. Could you add it?
>>
>> 2) A quick question about your example code: where would be the "logger"
>> object be created? Note that the implementation of this interface have to
>> give a non-param constructor, or as a static field of the class but in that
>> case you would not be able to log which instance is throwing this error (we
>> may have multiple producers within a single instance, even within a
>> thread). Just a reminder to consider in your implementation.
>>
>>
>> Guozhang
>>
>> On Tue, Dec 5, 2017 at 3:15 PM, Matthias J. Sax <matthias@confluent.io>
>> wrote:
>>
>>> Thanks a lot for the update! Great write-up! Very clearly explained what
>>> the change will look like!
>>>
>>> Looks good to me. No further comments from my side.
>>>
>>>
>>> -Matthias
>>>
>>>
>>> On 12/5/17 9:14 AM, Matt Farmer wrote:
>>>> I have updated this KIP accordingly.
>>>>
>>>> Can you please take a look and let me know if what I wrote looks
>> correct
>>> to
>>>> you?
>>>>
>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
>>> 210+-+Provide+for+custom+error+handling++when+Kafka+
>>> Streams+fails+to+produce
>>>>
>>>> Thanks!
>>>>
>>>> Matt
>>>>
>>>>
>>>> On December 4, 2017 at 9:39:13 PM, Matt Farmer (matt@frmr.me) wrote:
>>>>
>>>> Hey Matthias, thanks for getting back to me.
>>>>
>>>> That's fine. But if we add it to `test` package, we don't need to talk
>>>> about it in the KIP. `test` is not public API.
>>>>
>>>> Yes, that makes sense. It was in the KIP originally because I was, at
>> one
>>>> point, planning on including it. We can remove it now that we’ve
>> decided
>>> we
>>>> won’t include it in the public API.
>>>>
>>>> Understood. That makes sense. We should explain this clearly in the KIP
>>>> and maybe log all other following exceptions at DEBUG level?
>>>>
>>>>
>>>> I thought it was clear in the KIP, but I can go back and double check
>> my
>>>> wording and revise it to try and make it clearer.
>>>>
>>>> I’ll take a look at doing more work on the KIP and the Pull Request
>>>> tomorrow.
>>>>
>>>> Thanks again!
>>>>
>>>> On December 4, 2017 at 5:50:33 PM, Matthias J. Sax (
>>> matthias@confluent.io)
>>>> wrote:
>>>>
>>>> Hey,
>>>>
>>>> About your questions:
>>>>
>>>>>>> Acknowledged, so is ProducerFencedException the only kind of
>>> exception I
>>>>>>> need to change my behavior on? Or are there other types I need to
>>>> check? Is
>>>>>>> there a comprehensive list somewhere?
>>>>
>>>> I cannot think if any other atm. We should list all fatal exceptions
>> for
>>>> which we don't call the handler and explain why (exception is "global"
>>>> and will affect all other records, too | ProducerFenced is
>> self-healing).
>>>>
>>>> We started to collect and categorize exception here (not completed
>> yet):
>>>> https://cwiki.apache.org/confluence/display/KAFKA/
>>> Kafka+Streams+Architecture#KafkaStreamsArchitecture-TypesofExceptions
>>>> :
>>>>
>>>> This list should be a good starting point though.
>>>>
>>>>> I include it in the test package because I have tests that assert that
>>> if
>>>>> the record collector impl encounters an Exception and receives a
>>> CONTINUE
>>>>> that it actually does CONTINUE.
>>>>
>>>> That's fine. But if we add it to `test` package, we don't need to talk
>>>> about it in the KIP. `test` is not public API.
>>>>
>>>>> I didn't want to invoke the handler in places where the CONTINUE or
>> FAIL
>>>>> result would just be ignored. Presumably, after a FAIL has been
>>> returned,
>>>>> subsequent exceptions are likely to be repeats or noise from my
>>>>> understanding of the code paths. If this is incorrect we can revisit.
>>>>
>>>> Understood. That makes sense. We should explain this clearly in the KIP
>>>> and maybe log all other following exceptions at DEBUG level?
>>>>
>>>>
>>>> -Matthias
>>>>
>>>>
>>>> On 12/1/17 11:43 AM, Matt Farmer wrote:
>>>>> Bump! It's been three days here and I haven't seen any further
>> feedback.
>>>>> Eager to get this resolved, approved, and merged. =)
>>>>>
>>>>> On Tue, Nov 28, 2017 at 9:53 AM Matt Farmer <matt@frmr.me> wrote:
>>>>>
>>>>>> Hi there, sorry for the delay in responding. Last week had a holiday
>>> and
>>>>>> several busy work days in it so I'm just now getting around to
>>>> responding.
>>>>>>
>>>>>>> We would only exclude
>>>>>>> exception Streams can handle itself (like ProducerFencedException)
>> --
>>>>>>> thus, if the handler has code to react to this, it would not be bad,
>>> as
>>>>>>> this code is just never called.
>>>>>> [...]
>>>>>>> Thus, I am still in favor of not calling the
>>> ProductionExceptionHandler
>>>>>>> for fatal exception.
>>>>>>
>>>>>> Acknowledged, so is ProducerFencedException the only kind of
>> exception
>>> I
>>>>>> need to change my behavior on? Or are there other types I need to
>>> check?
>>>> Is
>>>>>> there a comprehensive list somewhere?
>>>>>>
>>>>>>> About the "always continue" case. Sounds good to me to remove it
>> (not
>>>>>>> sure why we need it in test package?)
>>>>>>
>>>>>> I include it in the test package because I have tests that assert
>> that
>>> if
>>>>>> the record collector impl encounters an Exception and receives a
>>> CONTINUE
>>>>>> that it actually does CONTINUE.
>>>>>>
>>>>>>> What is there reasoning for invoking the handler only for the first
>>>>>>> exception?
>>>>>>
>>>>>> I didn't want to invoke the handler in places where the CONTINUE or
>>> FAIL
>>>>>> result would just be ignored. Presumably, after a FAIL has been
>>> returned,
>>>>>> subsequent exceptions are likely to be repeats or noise from my
>>>>>> understanding of the code paths. If this is incorrect we can revisit.
>>>>>>
>>>>>> Once I get the answers to these questions I can make another pass on
>>> the
>>>>>> pull request!
>>>>>>
>>>>>> Matt
>>>>>>
>>>>>> On Mon, Nov 20, 2017 at 4:07 PM Matthias J. Sax <
>> matthias@confluent.io
>>>>
>>>>>> wrote:
>>>>>>
>>>>>>> Thanks for following up!
>>>>>>>
>>>>>>> One thought about an older reply from you:
>>>>>>>
>>>>>>>>>>> I strongly disagree here. The purpose of this handler isn't
>> *just*
>>>> to
>>>>>>>>>>> make a decision for streams. There may also be desirable side
>>>>>>> effects that
>>>>>>>>>>> users wish to cause when production exceptions occur. There may
>> be
>>>>>>>>>>> side-effects that they wish to cause when
>> AuthenticationExceptions
>>>>>>> occur,
>>>>>>>>>>> as well. We should still give them the hooks to preform those
>> side
>>>>>>> effects.
>>>>>>>
>>>>>>> And your follow up:
>>>>>>>
>>>>>>>>> - I think I would rather invoke it for all exceptions that could
>>>>>>> occur
>>>>>>>>> from attempting to produce - even those exceptions were returning
>>>>>>> CONTINUE
>>>>>>>>> may not be a good idea (e.g. Authorization exception). Until there
>>>>>>> is a
>>>>>>>>> different type for exceptions that are totally fatal (for example
>> a
>>>>>>>>> FatalStreamsException or some sort), maintaining a list of
>>>>>>> exceptions that
>>>>>>>>> you can intercept with this handler and exceptions you cannot
>> would
>>>>>>> be
>>>>>>>>> really error-prone and hard to keep correct.
>>>>>>>
>>>>>>> I understand what you are saying, however, consider that Streams
>> needs
>>>>>>> to die for a fatal exception. Thus, if you call the handler for a
>>> fatal
>>>>>>> exception, we would need to ignore the return value and fail -- this
>>>>>>> seems to be rather intuitive. Furthermore, users can register an
>>>>>>> uncaught-exception-handler and side effects for fatal errors can be
>>>>>>> triggered there.
>>>>>>>
>>>>>>> Btw: an AuthorizationException is fatal -- not sure what you mean by
>>> an
>>>>>>> "totally fatal" exception -- there is no superlative to fatal from
>> my
>>>>>>> understanding.
>>>>>>>
>>>>>>> About maintaining a list of exceptions: I don't think this is too
>>> hard,
>>>>>>> and users also don't need to worry about it IMHO. We would only
>>> exclude
>>>>>>> exception Streams can handle itself (like ProducerFencedException)
>> --
>>>>>>> thus, if the handler has code to react to this, it would not be bad,
>>> as
>>>>>>> this code is just never called. In case that there is an exception
>>>>>>> Streams could actually handle but we still call the handler (ie,
>> bug),
>>>>>>> there should be no harm either -- the handler needs to return either
>>>>>>> CONTINUE or FAIL and we would obey. It could only happen, that
>> Streams
>>>>>>> dies---as request by the user(!)---even if Streams could actually
>>> handle
>>>>>>> the error and move on. But this seems to be not a issue from my
>> point
>>> of
>>>>>>> view.
>>>>>>>
>>>>>>> Thus, I am still in favor of not calling the
>>> ProductionExceptionHandler
>>>>>>> for fatal exception.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> About the "always continue" case. Sounds good to me to remove it
>> (not
>>>>>>> sure why we need it in test package?) and to rename the "failing"
>>>>>>> handler to "Default" (even if "default" is less descriptive and I
>>> would
>>>>>>> still prefer "Fail" in the name).
>>>>>>>
>>>>>>>
>>>>>>> Last question:
>>>>>>>
>>>>>>>>> - Continue to *only* invoke it on the first exception that we
>>>>>>>>> encounter (before sendException is set)
>>>>>>>
>>>>>>>
>>>>>>> What is there reasoning for invoking the handler only for the first
>>>>>>> exception?
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> -Matthias
>>>>>>>
>>>>>>> On 11/20/17 10:43 AM, Matt Farmer wrote:
>>>>>>>> Alright, here are some updates I'm planning to make after thinking
>> on
>>>>>>> this
>>>>>>>> for awhile:
>>>>>>>>
>>>>>>>> - Given that the "always continue" handler isn't something I'd
>>>>>>> recommend
>>>>>>>> for production use as is, I'm going to move it into the test
>>>>>>> namespace and
>>>>>>>> remove it from mention in the public API.
>>>>>>>> - I'm going to rename the "AlwaysFailProductionExceptionHandler"
>> to
>>>>>>>> "DefaultProductionExceptionHandler"
>>>>>>>> - Given that the API for the exception handler involves being
>>>>>>> invoked by
>>>>>>>> streams to make a decision about whether or not to continue — I
>>>>>>> think that
>>>>>>>> we should:
>>>>>>>> - Continue to *only* invoke it on the first exception that we
>>>>>>>> encounter (before sendException is set)
>>>>>>>> - Stop invoking it for the self-healing fenced exceptions.
>>>>>>>> - I think I would rather invoke it for all exceptions that could
>>>>>>> occur
>>>>>>>> from attempting to produce - even those exceptions were returning
>>>>>>> CONTINUE
>>>>>>>> may not be a good idea (e.g. Authorization exception). Until there
>>>>>>> is a
>>>>>>>> different type for exceptions that are totally fatal (for example a
>>>>>>>> FatalStreamsException or some sort), maintaining a list of
>>>>>>> exceptions that
>>>>>>>> you can intercept with this handler and exceptions you cannot would
>>>>>>> be
>>>>>>>> really error-prone and hard to keep correct.
>>>>>>>> - I'm happy to file a KIP for the creation of this new Exception
>>>>>>> type
>>>>>>>> if there is interest.
>>>>>>>>
>>>>>>>> @ Matthias — What do you think about the above?
>>>>>>>>
>>>>>>>> On Tue, Nov 14, 2017 at 9:44 AM Matt Farmer <matt@frmr.me> wrote:
>>>>>>>>
>>>>>>>>> I responded before reading your code review and didn't see the bit
>>>>>>> about
>>>>>>>>> how ProducerFencedException is self-healing. This error handling
>>> logic
>>>>>>> is
>>>>>>>>> *quite* confusing to reason about... I think I'm going to sit down
>>>> with
>>>>>>>>> the code a bit more today, but I'm inclined to think that if the
>>>> fenced
>>>>>>>>> exceptions are, indeed, self healing that we still invoke the
>>> handler
>>>>>>> but
>>>>>>>>> ignore its result for those exceptions.
>>>>>>>>>
>>>>>>>>> On Tue, Nov 14, 2017 at 9:37 AM Matt Farmer <matt@frmr.me> wrote:
>>>>>>>>>
>>>>>>>>>> Hi there,
>>>>>>>>>>
>>>>>>>>>> Following up here...
>>>>>>>>>>
>>>>>>>>>>> One tiny comment: I would prefer to remove the "Always" from the
>>>>>>>>>> handler implementation names -- it sounds "cleaner" to me without
>>> it.
>>>>>>>>>>
>>>>>>>>>> Let me think on this. I generally prefer expressiveness to
>>>> clean-ness,
>>>>>>>>>> and I think that comes out in the names I chose for things. The
>>> straw
>>>>>>> man
>>>>>>>>>> in my head is the person that tried to substitute in the
>>>>>>> "AlwaysContinue"
>>>>>>>>>> variant and the "Always" is a trigger to really consider whether
>> or
>>>>>>> not
>>>>>>>>>> they *always* want to try to continue.
>>>>>>>>>>
>>>>>>>>>> To be truthful, after some thought, using the "AlwaysContinue"
>>>> variant
>>>>>>>>>> isn't something I'd recommend generally in a production system.
>>>>>>> Ideally
>>>>>>>>>> these handlers are targeted at handling a specific series of
>>>>>>> exceptions
>>>>>>>>>> that a user wants to ignore, and not ignoring all exceptions.
>> More
>>> on
>>>>>>> this
>>>>>>>>>> in a minute.
>>>>>>>>>>
>>>>>>>>>>> For the first category, it seems to not make sense to call the
>>>>>>> handle but
>>>>>>>>>> Streams should always fail. If we follow this design, the KIP
>>> should
>>>>>>> list
>>>>>>>>>> all exceptions for which the handler is not called.
>>>>>>>>>>
>>>>>>>>>> I strongly disagree here. The purpose of this handler isn't
>> *just*
>>> to
>>>>>>>>>> make a decision for streams. There may also be desirable side
>>> effects
>>>>>>> that
>>>>>>>>>> users wish to cause when production exceptions occur. There may
>> be
>>>>>>>>>> side-effects that they wish to cause when
>> AuthenticationExceptions
>>>>>>> occur,
>>>>>>>>>> as well. We should still give them the hooks to preform those
>> side
>>>>>>> effects.
>>>>>>>>>>
>>>>>>>>>> In light of the above, I'm thinking that the
>>>>>>>>>> "AlwaysContinueProductionExceptionHandler" variant should
>>> probably be
>>>>>>>>>> removed from the public API and moved into tests since that's
>> where
>>>>>>> it's
>>>>>>>>>> most useful. The more I think on it, the more I feel that having
>>> that
>>>>>>> in
>>>>>>>>>> the public API is a landmine. If you agree, then, we can rename
>> the
>>>>>>>>>> "AlwaysFailProductionExceptionHandler" to
>>>>>>>>>> "DefaultProductionExceptionHandler".
>>>>>>>>>>
>>>>>>>>>> Thoughts?
>>>>>>>>>>
>>>>>>>>>> On Fri, Nov 10, 2017 at 6:13 PM Matthias J. Sax <
>>>>>>> matthias@confluent.io>
>>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>>> I just review the PR, and there is one thing we should discuss.
>>>>>>>>>>>
>>>>>>>>>>> There are different types of exceptions that could occur. Some
>>> apply
>>>>>>> to
>>>>>>>>>>> all records (like Authorization exception) while others are for
>>>>>>> single
>>>>>>>>>>> records only (like record too large).
>>>>>>>>>>>
>>>>>>>>>>> For the first category, it seems to not make sense to call the
>>>> handle
>>>>>>>>>>> but Streams should always fail. If we follow this design, the
>> KIP
>>>>>>> should
>>>>>>>>>>> list all exceptions for which the handler is not called.
>>>>>>>>>>>
>>>>>>>>>>> WDYT?
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> -Matthias
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 11/10/17 2:56 PM, Matthias J. Sax wrote:
>>>>>>>>>>>> Just catching up on this KIP.
>>>>>>>>>>>>
>>>>>>>>>>>> One tiny comment: I would prefer to remove the "Always" from
>> the
>>>>>>>>>>> handler
>>>>>>>>>>>> implementation names -- it sounds "cleaner" to me without it.
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> -Matthias
>>>>>>>>>>>>
>>>>>>>>>>>> On 11/5/17 12:57 PM, Matt Farmer wrote:
>>>>>>>>>>>>> It is agreed, then. I've updated the pull request. I'm trying
>> to
>>>>>>> also
>>>>>>>>>>>>> update the KIP accordingly, but cwiki is being slow and
>> dropping
>>>>>>>>>>>>> connections..... I'll try again a bit later but please
>> consider
>>>> the
>>>>>>>>>>> KIP
>>>>>>>>>>>>> updated for all intents and purposes. heh.
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Sun, Nov 5, 2017 at 3:45 PM Guozhang Wang <
>>> wangguoz@gmail.com>
>>>>>>>>>>> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>> That makes sense.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Guozhang
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On Sun, Nov 5, 2017 at 12:33 PM, Matt Farmer <matt@frmr.me>
>>>>>>> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Interesting. I'm not sure I agree. I've been bitten many
>> times
>>>> by
>>>>>>>>>>>>>>> unintentionally shipping code that fails to properly
>> implement
>>>>>>>>>>> logging. I
>>>>>>>>>>>>>>> always discover this at the exact *worst* moment, too.
>>> (Normally
>>>>>>> at
>>>>>>>>>>> 3 AM
>>>>>>>>>>>>>>> during an on-call shift. Hah.) However, if others feel the
>>> same
>>>>>>> way
>>>>>>>>>>> I
>>>>>>>>>>>>>> could
>>>>>>>>>>>>>>> probably be convinced to remove it.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> We could also meet halfway and say that when a customized
>>>>>>>>>>>>>>> ProductionExceptionHandler instructs Streams to CONTINUE, we
>>> log
>>>>>>> at
>>>>>>>>>>> DEBUG
>>>>>>>>>>>>>>> level instead of WARN level. Would that alternative be
>>> appealing
>>>>>>> to
>>>>>>>>>>> you?
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On Sun, Nov 5, 2017 at 12:32 PM Guozhang Wang <
>>>>>>> wangguoz@gmail.com>
>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Thanks for the updates. I made a pass over the wiki again
>> and
>>>> it
>>>>>>>>>>> looks
>>>>>>>>>>>>>>>> good.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> About whether record collector should still internally log
>>> the
>>>>>>>>>>> error in
>>>>>>>>>>>>>>>> addition to what the customized ProductionExceptionHandler
>>>>>>> does. I
>>>>>>>>>>>>>>>> personally would prefer only to log if the returned value
>> is
>>>>>>> FAIL
>>>>>>>>>>> to
>>>>>>>>>>>>>>>> indicate that this thread is going to shutdown and trigger
>>> the
>>>>>>>>>>>>>> exception
>>>>>>>>>>>>>>>> handler.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Guozhang
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On Sun, Nov 5, 2017 at 6:09 AM, Matt Farmer <matt@frmr.me>
>>>>>>> wrote:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Hello, a bit later than I'd anticipated, but I've updated
>>> this
>>>>>>>>>>> KIP as
>>>>>>>>>>>>>>>>> outlined above. The updated KIP is now ready for review
>>> again!
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> On Sat, Nov 4, 2017 at 1:03 PM Matt Farmer <matt@frmr.me>
>>>>>>> wrote:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Ah. I actually created both of those in the PR and forgot
>>> to
>>>>>>>>>>>>>> mention
>>>>>>>>>>>>>>>> them
>>>>>>>>>>>>>>>>>> by name in the KIP! Thanks for pointing out the
>> oversight.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> I’ll revise the KIP this afternoon accordingly.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> The logging is actually provided for in the record
>>> collector.
>>>>>>>>>>>>>>> Whenever
>>>>>>>>>>>>>>>> a
>>>>>>>>>>>>>>>>>> handler continues it’ll log a warning to ensure that it’s
>>>>>>>>>>>>>>> *impossible*
>>>>>>>>>>>>>>>> to
>>>>>>>>>>>>>>>>>> write a handler that totally suppresses production
>>> exceptions
>>>>>>>>>>> from
>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>>>> log.
>>>>>>>>>>>>>>>>>> As such, the default continue handler just returns the
>>>>>>> continue
>>>>>>>>>>>>>>> value.
>>>>>>>>>>>>>>>> I
>>>>>>>>>>>>>>>>>> can add details about those semantics to the KIP as well.
>>>>>>>>>>>>>>>>>> On Sat, Nov 4, 2017 at 12:46 PM Matthias J. Sax <
>>>>>>>>>>>>>>> matthias@confluent.io
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> One more comment.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> You mention a default implementation for the handler
>> that
>>>>>>>>>>> fails. I
>>>>>>>>>>>>>>>>>>> think, this should be part of the public API and thus
>>> should
>>>>>>>>>>> have
>>>>>>>>>>>>>> a
>>>>>>>>>>>>>>>>>>> proper defined named that is mentioned in the KIP.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> We could also add a second implementation for the
>>>>>>>>>>> log-and-move-on
>>>>>>>>>>>>>>>>>>> strategy, as both are the two most common cases. This
>>> class
>>>>>>>>>>> should
>>>>>>>>>>>>>>>> also
>>>>>>>>>>>>>>>>>>> be part of public API (so users can just set in the
>>> config)
>>>>>>>>>>> with a
>>>>>>>>>>>>>>>>>>> proper name.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Otherwise, I like the KIP a lot! Thanks.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> -Matthias
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> On 11/1/17 12:23 AM, Matt Farmer wrote:
>>>>>>>>>>>>>>>>>>>> Thanks for the heads up. Yes, I think my changes are
>>>>>>> compatible
>>>>>>>>>>>>>>> with
>>>>>>>>>>>>>>>>>>> that
>>>>>>>>>>>>>>>>>>>> PR, but there will be a merge conflict that happens
>>>> whenever
>>>>>>>>>>> one
>>>>>>>>>>>>>>> of
>>>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>>>>>> PRs
>>>>>>>>>>>>>>>>>>>> is merged. Happy to reconcile the changes in my PR if
>>> 4148
>>>>>>> goes
>>>>>>>>>>>>>> in
>>>>>>>>>>>>>>>>>>> first. :)
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> On Tue, Oct 31, 2017 at 6:44 PM Guozhang Wang <
>>>>>>>>>>>>>> wangguoz@gmail.com
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> That sounds reasonable, thanks Matt.
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> As for the implementation, please note that there is
>>>>>>> another
>>>>>>>>>>>>>>>> ongoing
>>>>>>>>>>>>>>>>> PR
>>>>>>>>>>>>>>>>>>>>> that may touch the same classes that you are working
>> on:
>>>>>>>>>>>>>>>>>>>>> https://github.com/apache/kafka/pull/4148
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> So it may help if you can also take a look at that PR
>>> and
>>>>>>> see
>>>>>>>>>>>>>> if
>>>>>>>>>>>>>>> it
>>>>>>>>>>>>>>>>> is
>>>>>>>>>>>>>>>>>>>>> compatible with your changes.
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> Guozhang
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> On Tue, Oct 31, 2017 at 10:59 AM, Matt Farmer <
>>>>>>> matt@frmr.me>
>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> I've opened this pull request to implement the KIP as
>>>>>>>>>>>>>> currently
>>>>>>>>>>>>>>>>>>> written:
>>>>>>>>>>>>>>>>>>>>>> https://github.com/apache/kafka/pull/4165. It still
>>> needs
>>>>>>>>>>>>>> some
>>>>>>>>>>>>>>>>> tests
>>>>>>>>>>>>>>>>>>>>>> added,
>>>>>>>>>>>>>>>>>>>>>> but largely represents the shape I was going for.
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> If there are more points that folks would like to
>>>> discuss,
>>>>>>>>>>>>>>> please
>>>>>>>>>>>>>>>>> let
>>>>>>>>>>>>>>>>>>> me
>>>>>>>>>>>>>>>>>>>>>> know. If I don't hear anything by tomorrow afternoon
>>> I'll
>>>>>>>>>>>>>>> probably
>>>>>>>>>>>>>>>>>>> start
>>>>>>>>>>>>>>>>>>>>> a
>>>>>>>>>>>>>>>>>>>>>> [VOTE] thread.
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>>>> Matt
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> On Fri, Oct 27, 2017 at 7:33 PM Matt Farmer <
>>> matt@frmr.me
>>>>>>>>
>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> I can’t think of a reason that would be problematic.
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> Most of the time I would write a handler like this,
>> I
>>>>>>> either
>>>>>>>>>>>>>>> want
>>>>>>>>>>>>>>>>> to
>>>>>>>>>>>>>>>>>>>>>>> ignore the error or fail and bring everything down
>> so
>>>>>>> that I
>>>>>>>>>>>>>>> can
>>>>>>>>>>>>>>>>> spin
>>>>>>>>>>>>>>>>>>>>> it
>>>>>>>>>>>>>>>>>>>>>>> back up later and resume from earlier offsets. When
>> we
>>>>>>> start
>>>>>>>>>>>>>> up
>>>>>>>>>>>>>>>>> after
>>>>>>>>>>>>>>>>>>>>>>> crashing we’ll eventually try to process the message
>>> we
>>>>>>>>>>>>>> failed
>>>>>>>>>>>>>>> to
>>>>>>>>>>>>>>>>>>>>> produce
>>>>>>>>>>>>>>>>>>>>>>> again.
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> I’m concerned that “putting in a queue for later”
>>> opens
>>>>>>> you
>>>>>>>>>>>>>> up
>>>>>>>>>>>>>>> to
>>>>>>>>>>>>>>>>>>>>> putting
>>>>>>>>>>>>>>>>>>>>>>> messages into the destination topic in an unexpected
>>>>>>> order.
>>>>>>>>>>>>>>>> However
>>>>>>>>>>>>>>>>>>> if
>>>>>>>>>>>>>>>>>>>>>>> others feel differently, I’m happy to talk about it.
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> On Fri, Oct 27, 2017 at 7:10 PM Guozhang Wang <
>>>>>>>>>>>>>>>> wangguoz@gmail.com>
>>>>>>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> Please correct me if I'm wrong, but my
>> understanding
>>>> is
>>>>>>>>>>>>>> that
>>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>>>>>>>>> record
>>>>>>>>>>>>>>>>>>>>>>>>> metadata is always null if an exception occurred
>>> while
>>>>>>>>>>>>>> trying
>>>>>>>>>>>>>>>> to
>>>>>>>>>>>>>>>>>>>>>>>> produce.
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> That is right. Thanks.
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> I looked at the example code, and one thing I
>>> realized
>>>>>>> that
>>>>>>>>>>>>>>>> since
>>>>>>>>>>>>>>>>> we
>>>>>>>>>>>>>>>>>>>>> are
>>>>>>>>>>>>>>>>>>>>>>>> not passing the context in the handle function, we
>>> may
>>>>>>> not
>>>>>>>>>>>>>> be
>>>>>>>>>>>>>>>>>>>>> implement
>>>>>>>>>>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>>>>>>>>>>> logic to send the fail records into another queue
>> for
>>>>>>>>>>> future
>>>>>>>>>>>>>>>>>>>>> processing.
>>>>>>>>>>>>>>>>>>>>>>>> Would people think that would be a big issue?
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> Guozhang
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> On Thu, Oct 26, 2017 at 12:14 PM, Matt Farmer <
>>>>>>>>>>> matt@frmr.me
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> Hello all,
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> I've updated the KIP based on this conversation,
>> and
>>>>>>> made
>>>>>>>>>>>>>> it
>>>>>>>>>>>>>>> so
>>>>>>>>>>>>>>>>>>> that
>>>>>>>>>>>>>>>>>>>>>> its
>>>>>>>>>>>>>>>>>>>>>>>>> interface, config setting, and parameters line up
>>> more
>>>>>>>>>>>>>>> closely
>>>>>>>>>>>>>>>>> with
>>>>>>>>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>>>>>>>>>>>> interface in KIP-161 (deserialization handler).
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> I believe there are a few specific questions I
>> need
>>> to
>>>>>>>>>>>>>> reply
>>>>>>>>>>>>>>>>>>> to.....
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> The question I had about then handle parameters
>> are
>>>>>>>>>>> around
>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>>>>>>>>> record,
>>>>>>>>>>>>>>>>>>>>>>>>>> should it be `ProducerRecord<byte[], byte[]>`, or
>>> be
>>>>>>>>>>>>>>> generics
>>>>>>>>>>>>>>>> of
>>>>>>>>>>>>>>>>>>>>>>>>>> `ProducerRecord<? extends K, ? extends V>` or
>>>>>>>>>>>>>>>> `ProducerRecord<?
>>>>>>>>>>>>>>>>>>>>>>>> extends
>>>>>>>>>>>>>>>>>>>>>>>>>> Object, ? extends Object>`?
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> At this point in the code we're guaranteed that
>> this
>>>>>>> is a
>>>>>>>>>>>>>>>>>>>>>>>>> ProducerRecord<byte[], byte[]>, so the generics
>>> would
>>>>>>> just
>>>>>>>>>>>>>>> make
>>>>>>>>>>>>>>>>> it
>>>>>>>>>>>>>>>>>>>>>>>> harder
>>>>>>>>>>>>>>>>>>>>>>>>> to work with the key and value.
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> Also, should the handle function include the
>>>>>>>>>>>>>>> `RecordMetadata`
>>>>>>>>>>>>>>>> as
>>>>>>>>>>>>>>>>>>>>>> well
>>>>>>>>>>>>>>>>>>>>>>>> in
>>>>>>>>>>>>>>>>>>>>>>>>>> case it is not null?
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> Please correct me if I'm wrong, but my
>> understanding
>>>> is
>>>>>>>>>>>>>> that
>>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>>>>>>>>> record
>>>>>>>>>>>>>>>>>>>>>>>>> metadata is always null if an exception occurred
>>> while
>>>>>>>>>>>>>> trying
>>>>>>>>>>>>>>>> to
>>>>>>>>>>>>>>>>>>>>>>>> produce.
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> We may probably try to write down at least the
>>>>>>> following
>>>>>>>>>>>>>>>>> handling
>>>>>>>>>>>>>>>>>>>>>>>> logic
>>>>>>>>>>>>>>>>>>>>>>>>> and
>>>>>>>>>>>>>>>>>>>>>>>>>> see if the given API is sufficient for it
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> I've added some examples to the KIP. Let me know
>>> what
>>>>>>> you
>>>>>>>>>>>>>>>> think.
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> Cheers,
>>>>>>>>>>>>>>>>>>>>>>>>> Matt
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> On Mon, Oct 23, 2017 at 9:00 PM Matt Farmer <
>>>>>>> matt@frmr.me
>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> Thanks for this feedback. I’m at a conference
>> right
>>>>>>> now
>>>>>>>>>>>>>> and
>>>>>>>>>>>>>>> am
>>>>>>>>>>>>>>>>>>>>>>>> planning
>>>>>>>>>>>>>>>>>>>>>>>>> on
>>>>>>>>>>>>>>>>>>>>>>>>>> updating the KIP again with details from this
>>>>>>>>>>> conversation
>>>>>>>>>>>>>>>> later
>>>>>>>>>>>>>>>>>>>>>> this
>>>>>>>>>>>>>>>>>>>>>>>>> week.
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> I’ll shoot you a more detailed response then! :)
>>>>>>>>>>>>>>>>>>>>>>>>>> On Mon, Oct 23, 2017 at 8:16 PM Guozhang Wang <
>>>>>>>>>>>>>>>>> wangguoz@gmail.com
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>> Thanks for the KIP Matt.
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>> Regarding the handle interface of
>>>>>>>>>>>>>>>>> ProductionExceptionHandlerResp
>>>>>>>>>>>>>>>>>>>>>> onse,
>>>>>>>>>>>>>>>>>>>>>>>>>>> could
>>>>>>>>>>>>>>>>>>>>>>>>>>> you write it on the wiki also, along with the
>>> actual
>>>>>>>>>>>>>> added
>>>>>>>>>>>>>>>>> config
>>>>>>>>>>>>>>>>>>>>>>>> names
>>>>>>>>>>>>>>>>>>>>>>>>>>> (e.g. what
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
>>>>>>>>>>>>>>> 161%3A+streams+
>>>>>>>>>>>>>>>>>>>>>>>>> deserialization+exception+handlers
>>>>>>>>>>>>>>>>>>>>>>>>>>> described).
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>> The question I had about then handle parameters
>>> are
>>>>>>>>>>>>>> around
>>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>>>>>>>>>>> record,
>>>>>>>>>>>>>>>>>>>>>>>>>>> should it be `ProducerRecord<byte[], byte[]>`,
>> or
>>> be
>>>>>>>>>>>>>>> generics
>>>>>>>>>>>>>>>>> of
>>>>>>>>>>>>>>>>>>>>>>>>>>> `ProducerRecord<? extends K, ? extends V>` or
>>>>>>>>>>>>>>>> `ProducerRecord<?
>>>>>>>>>>>>>>>>>>>>>>>> extends
>>>>>>>>>>>>>>>>>>>>>>>>>>> Object, ? extends Object>`?
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>> Also, should the handle function include the
>>>>>>>>>>>>>>> `RecordMetadata`
>>>>>>>>>>>>>>>>> as
>>>>>>>>>>>>>>>>>>>>>>>> well in
>>>>>>>>>>>>>>>>>>>>>>>>>>> case it is not null?
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>> We may probably try to write down at least the
>>>>>>> following
>>>>>>>>>>>>>>>>> handling
>>>>>>>>>>>>>>>>>>>>>>>> logic
>>>>>>>>>>>>>>>>>>>>>>>>>>> and
>>>>>>>>>>>>>>>>>>>>>>>>>>> see if the given API is sufficient for it: 1)
>>> throw
>>>>>>>>>>>>>>> exception
>>>>>>>>>>>>>>>>>>>>>>>>> immediately
>>>>>>>>>>>>>>>>>>>>>>>>>>> to fail fast and stop the world, 2) log the
>> error
>>>> and
>>>>>>>>>>>>>> drop
>>>>>>>>>>>>>>>>> record
>>>>>>>>>>>>>>>>>>>>>> and
>>>>>>>>>>>>>>>>>>>>>>>>>>> proceed silently, 3) send such errors to a
>>> specific
>>>>>>>>>>>>>> "error"
>>>>>>>>>>>>>>>>> Kafka
>>>>>>>>>>>>>>>>>>>>>>>> topic,
>>>>>>>>>>>>>>>>>>>>>>>>>>> or
>>>>>>>>>>>>>>>>>>>>>>>>>>> record it as an app-level metrics (
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>> https://kafka.apache.org/documentation/#kafka_streams_
>>>>>>>>>>>>>>>>> monitoring
>>>>>>>>>>>>>>>>>>>>> )
>>>>>>>>>>>>>>>>>>>>>>>> for
>>>>>>>>>>>>>>>>>>>>>>>>>>> monitoring.
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>> Guozhang
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>> On Fri, Oct 20, 2017 at 5:47 PM, Matt Farmer <
>>>>>>>>>>>>>> matt@frmr.me
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>> I did some more digging tonight.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>> @Ted: It looks like the deserialization handler
>>>> uses
>>>>>>>>>>>>>>>>>>>>>>>>>>>> "default.deserialization.exception.handler"
>> for
>>> the
>>>>>>>>>>>>>>> config
>>>>>>>>>>>>>>>>>>>>>> name. No
>>>>>>>>>>>>>>>>>>>>>>>>>>>> ".class" on the end. I'm inclined to think this
>>>>>>> should
>>>>>>>>>>>>>> use
>>>>>>>>>>>>>>>>>>>>>>>>>>>> "default.production.exception.handler".
>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>> On Fri, Oct 20, 2017 at 8:22 PM Matt Farmer <
>>>>>>>>>>>>>> matt@frmr.me
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Okay, I've dug into this a little bit.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> I think getting access to the serialized
>> record
>>> is
>>>>>>>>>>>>>>>> possible,
>>>>>>>>>>>>>>>>>>>>>> and
>>>>>>>>>>>>>>>>>>>>>>>>>>> changing
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> the naming and return type is certainly
>> doable.
>>>>>>>>>>>>>> However,
>>>>>>>>>>>>>>>>>>>>>> because
>>>>>>>>>>>>>>>>>>>>>>>>> we're
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> hooking into the onCompletion callback we have
>>> no
>>>>>>>>>>>>>>> guarantee
>>>>>>>>>>>>>>>>>>>>>> that
>>>>>>>>>>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ProcessorContext state hasn't changed by the
>>> time
>>>>>>> this
>>>>>>>>>>>>>>>>>>>>>> particular
>>>>>>>>>>>>>>>>>>>>>>>>>>> handler
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> runs. So I think the signature would change to
>>>>>>>>>>>>>> something
>>>>>>>>>>>>>>>>>>>>> like:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ProductionExceptionHandlerResponse
>> handle(final
>>>>>>>>>>>>>>>>>>>>>>>> ProducerRecord<..>
>>>>>>>>>>>>>>>>>>>>>>>>>>>> record,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> final Exception exception)
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Would this be acceptable?
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> On Thu, Oct 19, 2017 at 7:33 PM Matt Farmer <
>>>>>>>>>>>>>>> matt@frmr.me>
>>>>>>>>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Ah good idea. Hmmm. I can line up the naming
>>> and
>>>>>>>>>>>>>> return
>>>>>>>>>>>>>>>> type
>>>>>>>>>>>>>>>>>>>>>> but
>>>>>>>>>>>>>>>>>>>>>>>>> I’m
>>>>>>>>>>>>>>>>>>>>>>>>>>> not
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> sure if I can get my hands on the context and
>>> the
>>>>>>>>>>>>>> record
>>>>>>>>>>>>>>>>>>>>>> itself
>>>>>>>>>>>>>>>>>>>>>>>>>>> without
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> other changes.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Let me dig in and follow up here tomorrow.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> On Thu, Oct 19, 2017 at 7:14 PM Matthias J.
>>> Sax <
>>>>>>>>>>>>>>>>>>>>>>>>>>> matthias@confluent.io>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Thanks for the KIP.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Are you familiar with KIP-161?
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
>>>>>>>>>>>>>>> 161%3A+streams+
>>>>>>>>>>>>>>>>>>>>>>>>>>>> deserialization+exception+handlers
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> I thinks, we should align the design
>>> (parameter
>>>>>>>>>>>>>> naming,
>>>>>>>>>>>>>>>>>>>>>> return
>>>>>>>>>>>>>>>>>>>>>>>>>>> types,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> class names etc) of KIP-210 to KIP-161 to
>> get
>>> a
>>>>>>>>>>>>>> unified
>>>>>>>>>>>>>>>>>>>>> user
>>>>>>>>>>>>>>>>>>>>>>>>>>>> experience.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> -Matthias
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> On 10/18/17 4:20 PM, Matt Farmer wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> I’ll create the JIRA ticket.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> I think that config name will work. I’ll
>>> update
>>>>>>> the
>>>>>>>>>>>>>>> KIP
>>>>>>>>>>>>>>>>>>>>>>>>>>> accordingly.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> On Wed, Oct 18, 2017 at 6:09 PM Ted Yu <
>>>>>>>>>>>>>>>>>>>>>> yuzhihong@gmail.com>
>>>>>>>>>>>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Can you create JIRA that corresponds to
>> the
>>>>>>> KIP ?
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> For the new config, how about naming it
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> production.exception.processor.class
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ? This way it is clear that class name
>>> should
>>>>>>> be
>>>>>>>>>>>>>>>>>>>>>> specified.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Cheers
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> On Wed, Oct 18, 2017 at 2:40 PM, Matt
>>> Farmer <
>>>>>>>>>>>>>>>>>>>>>> matt@frmr.me>
>>>>>>>>>>>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Hello everyone,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> This is the discussion thread for the
>> KIP
>>>>>>> that I
>>>>>>>>>>>>>>> just
>>>>>>>>>>>>>>>>>>>>>> filed
>>>>>>>>>>>>>>>>>>>>>>>>>>> here:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 210+-+Provide+for+custom+
>>>>>>>>>>>>>>> error+handling++when+Kafka+
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Streams+fails+to+produce
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Looking forward to getting some feedback
>>> from
>>>>>>>>>>>>>> folks
>>>>>>>>>>>>>>>>>>>>> about
>>>>>>>>>>>>>>>>>>>>>>>> this
>>>>>>>>>>>>>>>>>>>>>>>>>>> idea
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> and
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> working toward a solution we can
>>> contribute
>>>>>>> back.
>>>>>>>>>>>>>> :)
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Cheers,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Matt Farmer
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>> --
>>>>>>>>>>>>>>>>>>>>>>>>>>> -- Guozhang
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> --
>>>>>>>>>>>>>>>>>>>>>>>> -- Guozhang
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> --
>>>>>>>>>>>>>>>>>>>>> -- Guozhang
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> --
>>>>>>>>>>>>>>>> -- Guozhang
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> --
>>>>>>>>>>>>>> -- Guozhang
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>
>>>>
>>>
>>>
>>
>>
>> --
>> -- Guozhang
>>
> 


Mime
View raw message