Thank you Sophie and Bruno for finding a nice way forward keeping developers and users happy.
Updated the KIP to reflect the changes only from 4.0. Regards, Murali On Tue, Jul 2, 2024 at 8:18 AM Bruno Cadonna <cado...@apache.org> wrote: > Hi Sophie, > > I totally agree on evaluating the impact case by case! > > Nobody brushed any concerns under any rug. It was merely a concern about > a valid concern. I am sorry if it came across as brushing. > > Great that we found a viable solution! > > Best, > Bruno > > > > On 7/2/24 1:01 AM, Sophie Blee-Goldman wrote: > > 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 > >>>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>> > >>>>>>>>> > >>>>>>>> > >>>>>>> > >>>>>> > >>>>> > >>>> > >>> > >> > > >
