I think we should pause for a minute and have an honest conversation about whether the benefits of making this change outweigh the negatives. Here's my quick roundup of the positives and negatives, feel free to add to this list if there's else you think should be considered but then let's evaluate things.
Pros: -general agreement that the "default" prefix is inaccurate -one/two line change to update this code Cons: - A very large number of people set these configs and would all need to update their code (either when we eventually remove the old deprecated configs or right away to fix the deprecation warning. So while this is not a very "deep" disruption in that it's a small change, it's likely to be an extremely "wide" disruption in that many people are impacted - This change is objectively rather trivial, and people might be annoyed and/or confused as to why they need to make code changes for something so small - There seems to be no harm whatsoever done by the current state of things, as I have never once heard anyone complain or misinterpret the meaning of these configs due to the "default" prefix. So is this solving a problem that doesn't exist and just being pedantic in a way that is going to affect many users? I won't vote -1 on this and am happy to let it go through if others are strongly in favor. Just wanted to share my two cents and see if this resonates with anyone else. Because to me, it kind of feels like we're doing this to make ourselves happy, not our users. And I don't think we should be flippant about the user experience like this On Fri, Jun 14, 2024 at 3:39 PM Matthias J. Sax <mj...@apache.org> wrote: > > *`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 ? > > Yes, we should still rename it. I was just pointing out, that the KIP > does technically not need to mention this as all, as it's an internal > change. > > > I think you can start a VOTE for this KIP, too. > > > > -Matthias > > > On 6/14/24 1:35 PM, Muralidhar Basani wrote: > > 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 > >>>>>>>> > >>>>>>> > >>>>>> > >>>>> > >>>> > >>> > >> > > >
