tumblr / colossus

I/O and Microservice library for Scala
Apache License 2.0
1.14k stars 96 forks source link

Create a base trait for some built-in exceptions #594

Closed DanSimon closed 6 years ago

DanSimon commented 7 years ago

There are a bunch of exceptions that are mainly used to indicate an error state has occurred. DroppedReplyException,RequestTimeoutException, and ConnectionLostException are some examples. In some (maybe all?) of these cases, the stack trace is completely useless since the exceptions are always generated in the same spot and have no direct relevance to any user code.

We should figure out exactly which exceptions fall into this category and group them under some base type, something like ColossusRuntimeException.

dxuhuang commented 7 years ago

@DanSimon @benblack86 I'm thinking all subclasses of ServiceServerException and ServiceClientException that don't have a substantial message? This would be:

class RequestBufferFullException extends ServiceServerException("Request Buffer full")
class DroppedReplyException extends ServiceServerException("Dropped Reply")

class RequestTimeoutException extends ServiceClientException("Request Timed out")

There are other ServiceClientExceptions:

class ConnectionLostException(message: String) extends ServiceClientException(message)
class NotConnectedException(message: String) extends ServiceClientException(message)
class ClientOverloadedException(message: String) extends ServiceClientException(message)
cclass DataException(message: String) extends ServiceClientException(message)

and this one is not used anywhere:

class FatalServiceServerException(message: String) extends ServiceServerException(message)

Not all of these messages are relevant though? If this is the case for all or even most of them, we could just make ServiceServerException and ServiceClientException abstract classes and extend this new trait.

benblack86 commented 7 years ago

If we just want to log these exceptions without the stack trace, then we can add them to the config list.

If we wanted to do it more pragmatically, then having them extend a common trait/abstract class (i.e. ColossusRuntimeException), and matching on that would be useful. However, if we go for that approach, maybe we don't need to make it configurable. I'm leaning on removing the configuration since no one asked for it :)

DanSimon commented 7 years ago

So I believe we should have them extend ColossusRuntimeException regardless of how we decide to handle the logging. Just for better organization if nothing else. I'll probably have a PR ready soon that does this.

I'd ideally still like it to be configurable if those exceptions log stack traces, so I'm ok with keeping the configurable request formatter that was recently added. However I do agree that we should really start being more diligent with adding new features, and ensure that any new or changed functionality goes through the proper procedure of proposal and discussion, ideally through an RFC.

benblack86 commented 7 years ago

Ok, I'll assign this issue to you.