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
