The goal of this document is to define best practices for catching and throwing exceptions in the CDAP code base. Much of this will follow standard Java best practices.

Catching Exceptions

Avoid Log and Rethrow

This pattern should be avoided:

try {
  ...
} catch (IOException e) {
  LOG.warn("Unable to write to the data store", e);
  throw e;
}

The caller is forced to handle the exception thrown by this method and should be the one that decides whether to log or not.

This pattern causes duplicate log messages about the same error if the caller decides to log a message.

This pattern causes erroneous log messages when the exception is expected by the caller. For example, the caller may want to retry the method and only log a warning if all retries fail. If there is a log message in the method, there is no way to do that.

Be careful with catch(Exception e)

Checked exceptions usually are thrown to force a caller to respond to some situation from which they can recover, even when the caller is properly using the API. Unchecked exceptions usually are thrown because there is not a good way for the caller to respond to the error. Invalid use of the API falls under this category as well. Catching both checked and unchecked exceptions in the same catch block means you are responding to them both in the same way. This is usually not intended.

If you are trying to handle multiple checked exceptions in the same way, use multi-catch instead.

try {
  ...
} catch (SomeException | SomeOtherException e) {
  // handle them both the same way
}

InterruptedException

An InterruptedException is just another type of checked exception that a method can throw. This is most commonly thrown in the code base when some task is cancelled or a service is being shut down. As a general rule of thumb, a method should either propagate the exception up or it should stop whatever work it is doing. There are fewer situations where it makes sense to log the exception and continue. Think about what the method should do if it catches an InterruptedException because the service is being shut down.

Throwing Exceptions

Checked or Unchecked

From Effective Java:

[A checked exception] puts a nontrivial burden on the programmer. The burden is justified if the exceptional condition cannot be prevented by proper use of the API and the programmer using the API can take some useful action once confronted with the exception. Unless both of these conditions hold, an unchecked exception is more appropriate.

Javadocs

Every checked exception should be documented in the javadoc for the method using the @throws clause.

Many times, it makes sense to document the unchecked exceptions that can be thrown as well.

IllegalArgumentException

This is thrown when the method cannot be completed because the caller passed in an invalid argument. The method javadoc should make it clear what values are allowed. IllegalArgumentException is used for cases that the argument passed to a method is bad in a way that it can never be good (e.g. passing a negative number to a method that requires positive). 

IllegalStateException

This is thrown when the method cannot be completed because the state of the class is incorrect, even if there is no way to change the state. Changing the method arguments would never make the method complete successfully without a state change. 

RetryableException

This is a CDAP class that can be thrown when a method fails but can succeed if it is retried. This exception should not be thrown if there were any side-effects. The most common example of this is when a remote call is being made to a service that is currently unavailable.

Wrapping Exceptions

It often makes sense to translate one type of exception to another. For example, many HTTP handlers do this:

try {
  SomeEnum.valueOf(userProvidedInput);
} catch (IllegalArgumentException e) {
  throw new BadRequestException(e);
}

This is used when errors in some lower level abstraction need to be translated to errors in a higher level abstraction. In this case, it's used to tell the handler that it should return a 400 response code. In these situations, it is better to provide the error message as well as the cause instead of just providing the cause:

try {
  SomeEnum.valueOf(userProvidedInput);
} catch (IllegalArgumentException e) {
  throw new BadRequestException(e.getMessage(), e);
}

If an exception message is not specified, the exception classname will get pre-pended to the message. This is how java class names end up in UI error messages like:

IllegalArgumentException: xyz is not a supported type

instead of the preferred

xyz is not a supported type

Avoid throws Exception

If a method throws several types of checked exceptions, never consolidate them into a single 'throws Exception' declaration in an attempt to "simplify" the code. This makes it impossible for the caller to handle the individual exceptions correctly. If a method throws too many types of exceptions, this indicates that the method should perhaps be broken up into multiple methods, or some of those checked exceptions should be unchecked exceptions.

Do not declare that a method throws Exception in any class that is not meant to be implemented by user code. Throwing Exception means your callers are unable to handle your exception without catching other unrelated exceptions at the same time. It also makes it almost impossible to correctly determine if the exception should propagated or logged or retried or ignored.

One exception to this rule is for APIs that the platform defines for developers to implement, such as the pipeline plugin APIs or provisioner SPI. In these cases, it is often more friendly to let the implementation throw whatever exception they want. The platform must already handle any exception thrown by user code right when it calls it, and is responsible for translating it to another type of exception when it makes sense.

Another exception to this rule is for unit test cases, as those methods are not meant to be called by anyone except the test framework.

Avoid Throwables.propagate()

Throwables.propagate(t) will throw t as-is if it is a RuntimeException. If it is a checked exception, it will throw a RuntimeException with the checked exception as it's cause.

If the throwable is a RuntimeException that should be propagated, there should have been no reason to catch it to begin with.

If the throwable is a checked exception, it should probably be handled right there or it should be propagated as-is.

Throwables.propagate() is usually used as a lazy way "handle" exceptions. It usually ends up in ignoring an exception that should not be ignored.

Messages

Exception messages should contain all the information required by the caller to understand what caused the exception. The message should contain the values of all parameters and fields that contributed to the exception. For example, if the an exception occurred because an application could not be found, the application name should be included in the exception message. If a dataset could not be created, the message should include the dataset name. When possible, include any information the caller would find useful to avoid the exception. For example, if an invalid value is given, include what the valid values are.

Any exception thrown by an HTTP Handler should be written with a UI user in mind. It should be assumed that the exception message will show up in the UI. 

Overly long messages should be avoided. In particular, beware of messages that are generated based on data in some collection.

Messages must not contain sensitive information, such as secure keys.

Exceptions and State

When a method throws a checked exception, it should not leave things in an inconsistent state. Ideally, when an exception is thrown, state is left the same as if the method was never called.

Sometimes this is hard to avoid. For example, when a Table is truncated, it is first disabled, then dropped, then re-created. If the re-create fails due to some transient, the Table is left in an inconsistent state. In these situations, the method should at least make sure the inconsistent state can be fixed in some way. For example, if the Table is truncated again in this state, a blind call to disable the table will fail because it has already been dropped. Instead, the method should be sure to check the state of the table and only disable it if it exists and is enabled.