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 (m...@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 (matth...@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 <m...@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 <matth...@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 <m...@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 <m...@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 < >>>> matth...@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 <wangg...@gmail.com> >>>>>>>> wrote: >>>>>>>>>> >>>>>>>>>>> That makes sense. >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Guozhang >>>>>>>>>>> >>>>>>>>>>> On Sun, Nov 5, 2017 at 12:33 PM, Matt Farmer <m...@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 < >>>> wangg...@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 <m...@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 <m...@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 < >>>>>>>>>>>> matth...@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 < >>>>>>>>>>> wangg...@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 < >>>> m...@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 <m...@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 < >>>>>>>>>>>>> wangg...@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 < >>>>>>>> m...@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 < >>>> m...@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 < >>>>>>>>>>>>>> wangg...@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 < >>>>>>>>>>> m...@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 < >>>>>>>>>>> m...@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 < >>>>>>>>>>>> m...@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 < >>>>>>>>>>>>>>>>>>>>>>>> matth...@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 < >>>>>>>>>>>>>>>>>>> yuzhih...@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 < >>>>>>>>>>>>>>>>>>> m...@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 >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>>> >>>>> >>>> >>>> >> >
signature.asc
Description: OpenPGP digital signature
