Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FLINK-11134][rest] Do not log stacktrace handled exceptions #7346

Merged
merged 3 commits into from Jan 10, 2019

Conversation

zentol
Copy link
Contributor

@zentol zentol commented Dec 20, 2018

What is the purpose of the change

With this PR we do not log the full stacktrace for REST exception, under the conditions that

  1. DEBUG logging is disabled
  2. the exception is considered handled (i.e. is a RestHandlerException)

For these exceptions the exception message is usually sufficient to find out what's wrong; after all this is the only thing that users of the REST API get back as well.

As an opt-out mechanism DEBUG logging can be enabled, just to be safe.
Unhandled exceptions are excluded from this, as for these we cannot make any guarantee about the value of the error message.

Brief change log

  • modify AbstractRestHandler to only log the exception stack trace if debug logging is enabled
  • add logging of exceptions in AbstractHandler try-catch block
    • remove 2 redundant logging statements in the AbstractHandler

Verifying this change

This change is a trivial rework / code cleanup without any test coverage.

Copy link
Contributor

@tillrohrmann tillrohrmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing the problem of too verbose logging @zentol. I think there is also the AbstractTaskManagerFileHandler which logs the stack traces of all exceptions which we should adapt.

I'm wondering whether the logging of exceptions is not spread out across too many classes. It feels as if we have duplicated code a bit. I'm wondering whether we could not move the HandlerUtils.sendErrorResponse call to the AbstractHandler.java:161. WDYT?

@zentol
Copy link
Contributor Author

zentol commented Jan 3, 2019

Yeah the error handling is really all over the place.

I've consolidated it in AbstractHandler as you suggested.

Copy link
Contributor

@tillrohrmann tillrohrmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing my comments @zentol. +1 for merging after Travis gives green light.

if (throwable != null) {
handleException(ExceptionUtils.stripCompletionException(throwable), ctx, httpRequest);
}
finalizeRequestProcessing(finalUploadedFiles);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are slightly changing semantics here, because before we finalized the request processing after the error message has been sent. Now we send the error message and without waiting for the completion we finalize the request right away. One could change it to requestProcessingFuture.exceptionally(handleException).thenCompose(Function.identity).whenComplete(). But I think it should also be ok to finalize the request right away since we only send an error message and whether the error message arrives or the client connection is terminated via a ConnectionException should not matter too much.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made changes to ensure that we finalize the request after the error response was sent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants