*`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
