Status

Current state: Under Discussion

Discussion thread: https://lists.apache.org/thread/5gmns2pfw1gb0jvlrj13q7jd50x764x2

JIRA: TODO

Motivation

Java has a convention where classes extending Throwable accept the cause of the Throwable as a second argument to the constructor. org.apache.kafka.common.config.ConfigException  however does not follow that convention as it has a constructor ConfigException(String name, Object value), which delegates to ConfigException(String name, Object value, String message) that in turn delegates to KafkaException(String message) . This API is brittle as developers often misuse the constructor with the intention of passing a cause as the second argument which never makes it to the stacktrace [1][2][3][4]. The cause and the throwable chain often proves useful in debugging the misconfiguration. For instance, when debugging incorrectly configured TLS in [1], it can help identify the cause of misconfiguration.


This KIP proposes the introduction of another constructor ConfigException(String message, Throwable cause) to adhere to the convention and the addition of another constructor to capture cause.

Public Interfaces

ConfigException.java
// Deprecate existing constructor
@Deprecated
public ConfigException(String name, Object value) {
	this(name, value, null);
}

// New Constructor
public ConfigException(String message, Throwable cause) {
	super(message, cause);
}

// New Constructor
public ConfigException(String name, Object value, String message, Throwable cause) {
	super("Invalid value " + value + " for configuration " + name + (message == null ? "" : ": " + message), cause);
}

// Update existing constructor
public ConfigException(String name, Object value, String message) {
	this(name, value, message, null);
}


Proposed Changes


We deprecate ConfigException(String name, Object value)   and introduce ConfigException(String message, Throwable cause) . We also update existing usages of the former constructor as it will be marked deprecated.


We also add ConfigException(String name, Object value, String message, Throwable cause) and make ConfigException(String name, Object value, String message)  delegate to it. This will be useful in scenarios where we use the latter when catching an exception so as to not lose the underlying cause.

Compatibility, Deprecation, and Migration Plan


JLS §15.12.2.5 [5] ensures the more specific constructor is chosen. Caution must be taken when passing null as the value of the second argument as the constructor with Throwable as the second argument would be invoked since it's more specific [6].

Test Plan


The compiler should help us ensure the change is safe. Existing unit tests that assert on exception message shouldn't break.

Rejected Alternatives


  • Adding a cause using Throwable#initCause  before throwing the exception at the point of instantiation. This may work for existing usages of the constructor but doesn't save future developers from running into the pitfall.
  • No labels