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