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

Reply via email to