Gaurav, thanks for the KIP. DA1: The four examples you give of improper usage of the existing constructor are all Kafka usages, so arguably they are just bugs. Do we know if ConfigException can be created/thrown by 3rd party code via our pluggable interfaces? If there are any interface usages, can you list them in the KIP?
DA2: Seems fine to keep the (String, Object) usage. Thanks! David A On Mon, Jan 27, 2025 at 9:36 PM Greg Harris <greg.har...@aiven.io.invalid> wrote: > Hi Gaurav, > > Thanks for the KIP! > > I've noticed the lack of Throwable-based constructors in the past, so I'm > glad you're bringing this up for discussion. I think it makes sense to be > able to add more information relevant for debugging configuration issues > directly as a cause. > > 1. Could you discuss #addSuppressed as a rejected alternative? > 2. Since ConfigException has not had any causes until now, there may be > implementations that don't inspect the cause or evaluate the stack > trace [1]. Do you plan on changing any of these to aid debugging? > > > Of the 12 usages of ConfigException > > 3. I am skeptical of characterizing the usages of ConfigException in this > way. Because it is a public API, there are likely hundreds to thousands of > usages in downstream projects that a search of the main Kafka repository > will not uncover. > 4. The ConfigDef.Validator interface accepts (String, Object) [2], so it > could be natural for implementations to use the (String, Object) > constructor. A constructor removal would force rework of these > implementations to either add a meaningful message, or churn to add an > explicit `null` message. I'm unsure if that is a net positive for the > ecosystem, especially when the (String, Object, String) constructor is > already present. > > > The last one [3] uses the name argument as a message which seems wrong > too. > > That one is definitely a typo, as it's using logger syntax in an exception > argument. It's also a default branch in an enum switch, so it's probably > never been exercised since being written. I wouldn't use it to draw broader > conclusions here. > > Thanks, > Greg > > [1] > > https://github.com/apache/kafka/blob/dc396f47e8f7b82eef8b33c7a6153f5ed3a1127f/clients/src/main/java/org/apache/kafka/common/config/ConfigDef.java#L634 > [2] > > https://github.com/apache/kafka/blob/dc396f47e8f7b82eef8b33c7a6153f5ed3a1127f/clients/src/main/java/org/apache/kafka/common/config/ConfigDef.java#L948 > > On Mon, Jan 27, 2025 at 5:45 PM Gaurav Narula <ka...@gnarula.com> wrote: > > > Thanks for your feedback Chia! Please find my answers inline > > > > > On 27 Jan 2025, at 13:32, Chia-Ping Tsai <chia7...@apache.org> wrote: > > > > > > hi Gaurav > > > > > > Thanks for this KIP as I totally agree that `cause` can offer more > > useful information. Please take a look at following small questions. > > > > > > chia_0: do we need to deprecate existent constructor? It seems to me > > that is not conflict to the new one. > > > > While I don't think there's a requirement to deprecate the constructor, I > > propose it because it's quite an unexpected signature and felt it's > better > > to be explicit with ConfigException(String name, Object value, String > > message). > > > > Of the 12 usages of ConfigException(String name, Object value), 3 of them > > use an Object as the value, 2 of which are null [1][2]. The last one [3] > > uses the name argument as a message which seems wrong too. > > > > > > > > chia_1: Have you consider adding `ConfigException(String name, Object > > value, String message, Throwable cause)`? it is used widely too. > > > > Thanks for pointing that out. I do see quite a few usages where we catch > > an exception and throw ConfigException(String name, Object value, String > > message) only to discard the cause. I agree passing the cause would be of > > use. I've updated the KIP to reflect this. > > > > Best, > > Gaurav > > > > [1] > > > https://github.com/apache/kafka/blob/dc396f47e8f7b82eef8b33c7a6153f5ed3a1127f/raft/src/main/java/org/apache/kafka/raft/QuorumConfig.java#L272 > > [2] > > > https://github.com/apache/kafka/blob/dc396f47e8f7b82eef8b33c7a6153f5ed3a1127f/raft/src/main/java/org/apache/kafka/raft/QuorumConfig.java#L292 > > [3] > > > https://github.com/apache/kafka/blob/dc396f47e8f7b82eef8b33c7a6153f5ed3a1127f/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/errors/RetryWithToleranceOperator.java#L282 > > > > > > > > thanks, > > > Chia-Ping > > > > > > On 2025/01/26 11:08:59 Gaurav Narula wrote: > > >> Hi Everyone, > > >> > > >> I'd like to initiate a discussion on KIP-1129: Update ConfigException > > constructors at https://cwiki.apache.org/confluence/x/n4pEF. > > >> > > >> This KIP adds a constructor to ConfigException to accept a Throwable > as > > a second argument following the Java convention for Throwables and > > deprecates the existing constructor that accepts an Object as the second > > argument. > > >> > > >> Looking forward to hearing the community's feedback on this! > > >> > > >> Regards, > > >> Gaurav > > > > > -- David Arthur
