Thanks Bruno -- I think shipping this with 4.0 when most users will expect to need code changes anyway is a good compromise. I'm happy to change my position and cast a +1 vote on this. I suppose we will just need to wait a few weeks to actually merge these changes, given the interim short-cycle 3.9 release we're doing before 4.0?
However, I do want to address this: what I do not like about the > concern is that it blocks us from ever changing low priority stuff in > the code base. Which IMO is bad from a maintainability perspective. > First off, I wholeheartedly agree that we should not block changes just because they are low priority. I hope that goes without saying :) But my position was not "we should never do anything that disrupts users unless it's high priority" -- all I'm saying is that "we should evaluate each proposal and weigh the potential benefits against the potential cost and/or risks", and take things on a case by case basis, which I hope we can all agree on. In sum: we should be careful not to put undue burden on our users just to make ourselves happy. It's up to us to have that discussion each time and I don't think it's fair to brush the concerns about a specific case under the rug in the name of not blocking low-priority changes in the future. Anyways, I'll follow up on the vote thread to give this a +1. Whoever reviews this, please take care to time the merging of any PRs around (ie after) the 3.9 branch cut Thanks all, Sophie On Mon, Jul 1, 2024 at 2:28 AM Bruno Cadonna <cado...@apache.org> wrote: > Hi Sophie, > > Thank you for your analysis! > > I understand your valid concerns. However, what I do not like about the > concern is that it blocks us from ever changing low priority stuff in > the code base. Which IMO is bad from a maintainability perspective. > > I agree that the change makes primarily us happy in short-term, but more > precise naming makes our users happy as well in the long run, expecially > the ones that newly join our otter-club. > > I also think that people that set their builds to fail for deprecated > parts, expect to change code when they use a new version of Kafka > Streams. The users that do not let their builds fail for deprecations > have to change their code anyways when they switch to a new major release. > > I empathize with the effort to minimize the work for our users, so my > proposal would be to ship this KIP with a release where we have other > wide radius deprecations and changes that our users need to do. For > example, we could ship the deprecated property names with 4.0. I guess > users need to change a couple of things in their code when they switch > to 4.0, so those deprecations will carry (almost) no weight. > > What do you think? > > Best, > Bruno > > > On 6/15/24 8:58 AM, Sophie Blee-Goldman wrote: > > 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 > >>>>>>>>>> > >>>>>>>>> > >>>>>>>> > >>>>>>> > >>>>>> > >>>>> > >>>> > >>> > >> > > >
