Thanks Matthias and Nick. Regarding the prefix "uncaught." is fine with me too, but any other opinions on this one ?
*101 follow-up:* *Given that `DESERIALIZATION_EXCEPTION_HAN**DLER_CLASS_DOC` is public we* *cannot just rename it, but must follow the same deprecation pattern as* *for the config name itself. (It seems it was added as public by mistake,* *and the new variable for the _DOC string should be private...)* Agree, as DESERIALIZATION_EXCEPTION_HANDLER_CLASS_DOC is public, we shall deprecate it. Updated kip. *`DEFAULT_PRODUCTION_EXCEPTION_HANDLER_CLASS_DOC` is private and does notneed to be covered in the KIP technically -- it's ok to just leave it inof course.* Yea, technically it has no implications, but as we are anyways touching code around this, could it be better to rename ? *102 follow-up:I believe for other config, we actually use the new config if it's set,and only fall back to the old config if the new one is not set. If bothare set, the new one wins and we just log a WARN that the old one isignored in favor of the new one. -- It might be best to stick theestablished pattern?* Agree, we should follow the same pattern. Updated kip. Thanks, Murali On Fri, Jun 14, 2024 at 3:12 AM Matthias J. Sax <mj...@apache.org> wrote: > Using `uncaught.` prefix would be fine with me. (Even if I find it a > little be redundant personally?) > > > > 101 follow-up: > > Given that `DESERIALIZATION_EXCEPTION_HANDLER_CLASS_DOC` is public we > cannot just rename it, but must follow the same deprecation pattern as > for the config name itself. (It seems it was added as public by mistake, > and the new variable for the _DOC string should be private...) > > -> actually compare KIP-1053 (old incorrect number 1052) "Align the > naming convention for config and default variables in *Config classes" > for a related discussion. > > > `DEFAULT_PRODUCTION_EXCEPTION_HANDLER_CLASS_DOC` is private and does not > need to be covered in the KIP technically -- it's ok to just leave it in > of course. > > > > > 102 follow-up: > > > But if user sets both old and new configs, a ConfigException to be > thrown. > > I believe for other config, we actually use the new config if it's set, > and only fall back to the old config if the new one is not set. If both > are set, the new one wins and we just log a WARN that the old one is > ignored in favor of the new one. -- It might be best to stick the > established pattern? > > > > -Matthias > > > > > On 6/13/24 3:14 AM, Nick Telford wrote: > > Hi everyone, > > > > On a semantic note, would it perhaps make more sense to rename them " > > uncaught." instead? These handlers are essentially the "last resort" > > exception handlers, because Exceptions can be caught *within* a > component, > > e.g. a Deserializer can catch and handle an exception without the > > configured default.deserialization.exception.handler being invoked. > > > > I think this would better align these with JVM norms, e.g. > > Thread#setUncaughtExceptionHandler, which behaves the same (albeit > > configured through code). > > > > Regards, > > Nick > > > > On Thu, 13 Jun 2024 at 10:22, Muralidhar Basani > > <muralidhar.bas...@aiven.io.invalid> wrote: > > > >> Thanks Matthias and Greg. > >> > >> Have updated the KIP based on Matthias's comments. > >> > >>>> 100: Config names are part of the public interface, and the KIP should > >>>> not say "none" in this section, but call out which configs are > >>>> deprecated and which ones are newly added. > >> > >> Updated kip. > >> > >> > >>>> 101: Nit. In "Propose Changes" there is the template placeholder text > >>>> > >>>>> Describe the new thing you want to do in appropriate detail. This may > >> be > >>>> fairly extensive and have large subsections of its own. Or it may be a > >> few > >>>> sentences. Use judgement based on the scope of the change. > >>>> > >>>> Similarly in "Test Plan" section > >>>> > >> > >> Updated kip. > >> > >> > >>>> 102: The "Deprecation" section should explain the behavior if both, > old > >>>> and new configs, are set. > >> > >> Updated kip. I think a ConfigException has to be thrown here if both > >> configs are set. > >> > >> Thanks, > >> Murali > >> > >> On Thu, Jun 13, 2024 at 2:28 AM Matthias J. Sax <mj...@apache.org> > wrote: > >> > >>> Thanks Greg, this is a valid idea. > >>> > >>> However, there was no demand in the past to allow for error handler > >>> callbacks in a fine grained manner, and I am frankly also not sure if > it > >>> would make sense or would be required. > >>> > >>> Thus, I am not concerned about this case, and believe this KIP does > make > >>> sense. > >>> > >>> Happy to change my mind if somebody has a different opinion. We could > >>> re-purpose this KIP to add per-topic error handler, too. > >>> > >>> > >>> > >>> -Matthias > >>> > >>> On 6/12/24 1:11 PM, Greg Harris wrote: > >>>> Hi Murali, > >>>> > >>>> Thanks for the KIP! > >>>> > >>>> I'm not familiar with Streams so I'll pose a general question, open > for > >>>> anyone to answer: > >>>> > >>>> The configs that are being changed don't currently accept in-place > >>>> overwrites in the code, so the "default.*" prefix doesn't make sense. > >>> Could > >>>> there be a KIP to accept in-place overwrites in the future, such that > >> the > >>>> "default.*" prefix would make sense? > >>>> If so, this KIP would make that other KIP harder to implement, as we > >>> would > >>>> have already recommended everyone to move off of the "default.* > prefix. > >>> Or > >>>> to put it another way, this KIP closes doors rather than opening them. > >>>> > >>>> Or at least that's how it looks to a Streams outsider. I'm happy to > >> defer > >>>> to the experts in this case :) > >>>> > >>>> Thanks, > >>>> Greg > >>>> > >>>> On Mon, Jun 10, 2024 at 3:47 PM Matthias J. Sax <mj...@apache.org> > >>> wrote: > >>>> > >>>>> Thanks for the KIP Murali, > >>>>> > >>>>> Overall LGTM. A few comments. > >>>>> > >>>>> > >>>>> > >>>>> 100: Config names are part of the public interface, and the KIP > should > >>>>> not say "none" in this section, but call out which configs are > >>>>> deprecated and which ones are newly added. > >>>>> > >>>>> > >>>>> 101: Nit. In "Propose Changes" there is the template placeholder text > >>>>> > >>>>>> Describe the new thing you want to do in appropriate detail. This > may > >>> be > >>>>> fairly extensive and have large subsections of its own. Or it may be > a > >>> few > >>>>> sentences. Use judgement based on the scope of the change. > >>>>> > >>>>> Similarly in "Test Plan" section > >>>>> > >>>>> Please remove both :) > >>>>> > >>>>> > >>>>> 102: The "Deprecation" section should explain the behavior if both, > >> old > >>>>> and new configs, are set. > >>>>> > >>>>> > >>>>> Thanks a lot! > >>>>> > >>>>> > >>>>> -Matthias > >>>>> > >>>>> > >>>>> On 6/9/24 9:30 PM, Muralidhar Basani wrote: > >>>>>> Hello all, > >>>>>> > >>>>>> With this KIP > >>>>>> < > >>>>> > >>> > >> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-1056%3A+Remove+%60default.%60+prefix+for+exception+handler+StreamsConfig > >>>>>> , > >>>>>> I would like to mention that a couple of exception handler configs > in > >>>>>> StreamsConfig are defined as default configs, despite having no > >>>>> alternative > >>>>>> values. Hence would propose to deprecate them and introduce new > >> configs > >>>>>> without the 'default.' prefix. > >>>>>> > >>>>>> This KIP is briefly discussed here in the jira KAFKA-16853 > >>>>>> <https://issues.apache.org/jira/browse/KAFKA-16863> too. > >>>>>> > >>>>>> I would appreciate any feedback or suggestions you might have. > >>>>>> > >>>>>> Thanks, > >>>>>> Murali > >>>>>> > >>>>> > >>>> > >>> > >> > > >
