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

[#1731] Improve error handling in onApplicationStop() method #664

Closed
wants to merge 2 commits into from
Closed

Conversation

nemecec
Copy link
Contributor

@nemecec nemecec commented Sep 24, 2013

If some plugin throws exception in onApplicationStop() method, onApplicationStop() is not called for other plugins.
Instead, we should log it and continue to call other plugins.

If some plugin throws exception in onApplicationStop() method, onApplicationStop() is not called for other plugins.
Instead, we should log it and continue to call other plugins.
@cloudbees-pull-request-builder

play-1-3-x-pull-requests #53 FAILURE
Looks like there's a problem with this pull request

@nemecec
Copy link
Contributor Author

nemecec commented Sep 24, 2013

Jenkins failure does not seem to be related to my change.

@Notalifeform
Copy link
Contributor

indeed, must have been a hickup. could you a lighthouse request for this issue|?

Otherwise the error message (at INFO level) is rather useless.
@cloudbees-pull-request-builder

play-1-3-x-pull-requests #63 FAILURE
Looks like there's a problem with this pull request

@nemecec
Copy link
Contributor Author

nemecec commented Sep 30, 2013

Again, Jenkins failure does not seem to be related to my change. (the same problem is still there, as before?)

@Notalifeform
Copy link
Contributor

yep.. weird. I can't imagime how your change could cause it.. could you add a lighthouse issue?

@Notalifeform
Copy link
Contributor

ah, just realized @pepite did some work to make 1.3.x work on cloudbees - these chnages are not in 1.2.x, so that explains it.

@nemecec
Copy link
Contributor Author

nemecec commented Sep 30, 2013

I did add a lighthouse issue. In case you missed it, here it is: http://play.lighthouseapp.com/projects/57987/tickets/1731-improve-error-handling-in-onapplicationstop-method

@Notalifeform
Copy link
Contributor

ah. I missed it. thanks.

if (t.getMessage() == null)
Logger.error(t, "Error while stopping %s", plugin);
else if (Logger.isDebugEnabled())
Logger.debug(t, "Error while stopping %s", plugin);
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why you put different level of Log for the same exception, I think you can use error level

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When shutting down, there can be all kinds of exceptions flying around and most of them are not interesting - so I log the stack trace only if the exception message is missing (e.g. NPE) or if DEBUG level is enabled. In short: this reduces the verbosity of the logs.

Copy link
Contributor

Choose a reason for hiding this comment

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

though not very intuitive, it seems reasonable. I'll merge it.

@Notalifeform
Copy link
Contributor

merged. thanks.

@nemecec nemecec deleted the patch-1 branch October 7, 2013 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants