feat(retrofit): add SpinnakerServerExceptionHandler #4524
Conversation
…s a RetrofitError with a fix of a corner case in RetrofitExceptionHandler along the way. This paves the way to demonstrate similar behavior when a task throws a SpinnakerServerException
…nto BaseRetrofitExceptionHandler to pave the way to share it with a new SpinnakerServerExceptionHandler class
…o take an Exception instead of only RetrofitError The logic in the implementation is still specific to retrofit as it looks for retrofit-specific annotations in the stack trace. This way though, SpinnakerNetworkExceptions caused by e.g. IOExceptions (e.g. with no underlying RetrofitError in the causal chain) are considered retryable when they happen in an idempotent retrofit request.
… kind argument to a String which decouples children of BaseRetrofitExceptionHandler from retrofit1. This makes it easier to remove retrofit1 from the classpath once we move to retrofit2.
to provide similar functionality when tasks throw SpinnakerServerException that RetrofitExceptionHandler provides for RetrofitException. Specifically, this means: * retry behavior is the same with SpinnakerServerExceptionHandler as it is with RetrofitExceptionHandler * the information in the pipeline execution context is similar * the way errors appear in the UI with SpinnakerSeverExceptionHandler is a combination of DefaultExceptionHandler, which is an improvement over RetrofitExceptionHandler (e.g. http response codes and URLs are present)
so there's a SpinnakerServerExceptionHandler available to RunTaskHandler
|
We prefer that non-test backend code be written in Java or Kotlin, rather than Groovy. The following files have been added and written in Groovy:
See our server-side conventions here. |
| findHttpMethodAnnotation(e) in ["GET", "HEAD", "DELETE", "PUT"] | ||
| } | ||
|
|
||
| private static String findHttpMethodAnnotation(Exception exception) { |
There was a problem hiding this comment.
Only worry, just at a glance here... there's a fair bit of reflection on this that I've seen cause major perf issues. This particular code doesn't look bad off hand ... just one of those triggering a "caution" flag. I'd wonder about using AnnotationUtils or similar spring libs which as i recall do caching of reflection calls so the forName/method search is only done once.
There was a problem hiding this comment.
That said... it IS a "move" soo.... ignore for now, just one of those caught my eye
There was a problem hiding this comment.
Yup, it's a drag, but it's no more reflection than we were already doing. I don't think we need it once we move to retrofit2.
Not used yet, since orca doesn't use SpinnakerRetrofitErrorHandler, and doesn't explicitly throw SpinnakerServerException, nor its children. Once we do use it, we'll get similar information present in the execution context if a SpinnakerServerException happens as we get today if a RetrofitError happens, and the same retry behavior.