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