You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by DaanHoogland <gi...@git.apache.org> on 2015/07/29 22:56:18 UTC
[GitHub] cloudstack pull request: Cloudstack 8656
GitHub user DaanHoogland opened a pull request:
https://github.com/apache/cloudstack/pull/637
Cloudstack 8656
filling empty catch clauses with log messages
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/DaanHoogland/cloudstack CLOUDSTACK-8656
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/637.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #637
----
commit 483a8edf83977b0469bb4d951ded69170719fd5a
Author: Daan Hoogland <da...@gmail.com>
Date: 2015-07-21T11:10:15Z
CLOUDSTACK-8656: filling empty catch block with info messages
using regexp "catch\s*\(\s*(Exception|Throwable)\s*\w*\)\s*\{\s*\}"
commit 7d3d82048c315c00a1e592ebff88e8c12550554d
Author: Daan Hoogland <da...@onecht.net>
Date: 2015-07-29T20:52:12Z
CLOUDSTACK-8656: info messgage on interupted exceptions
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack 8656 adding messages to empty ...
Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on the pull request:
https://github.com/apache/cloudstack/pull/637#issuecomment-126204154
Fair enough, @DaanHoogland
Thanks for the exaplanation. :)
LGTM :+1:
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack 8656 adding messages to empty ...
Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on the pull request:
https://github.com/apache/cloudstack/pull/637#issuecomment-126215256
@kishankavala @DaanHoogland :+1: for the Jira ticket.
After 4.6 we can come back to this and assess the messages accordingly. But would be nice to have the logs in systems running 4.6.
Cheers,
Wilder
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack 8656 adding messages to empty ...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/637#issuecomment-126260440
@DaanHoogland Merge this in 4.6. If possible push the log level changes in 4.6 as well.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack 8656 adding messages to empty ...
Posted by kishankavala <gi...@git.apache.org>.
Github user kishankavala commented on the pull request:
https://github.com/apache/cloudstack/pull/637#issuecomment-126205414
@DaanHoogland
Agreed that setting appropriate log level is out of the scope for this PR. But we should file a jira ticket to track and address it before next release.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack 8656 adding messages to empty ...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/637#issuecomment-126206244
@kishankavala you actually made me think I should wait till after 4.6, on the other hand we will have to take the pain in a release.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack 8656 adding messages to empty ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/637
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack 8656 adding messages to empty ...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the pull request:
https://github.com/apache/cloudstack/pull/637#issuecomment-126214607
Code wise it is LGTM
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack 8656 adding messages to empty ...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/637#issuecomment-126331708
So I will amend the interrupted to be level debug and then merge. next I will create a new PR for any remaining empty catch blocks, @koushik-das.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack 8656 adding messages to empty ...
Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on the pull request:
https://github.com/apache/cloudstack/pull/637#issuecomment-126200897
Hi @DaanHoogland
Nice you took time to have a look at those. It really helps sys admins to know what is going on with ACS. However, I would rather have them logged as WARN over INFO. Why? Because if we fall into the catch block, is because something went wrong or might probably require manual intervention. If you agree, would that be a lot of work to change them into WARN?
Concerning the logs getting filled up too soon, I wouldn't really bother. From my point of view that's an admin task to have a clever rotation of logs. If ACS code base was more robust, we could afford less logs and filling up files wouldn't be a problem. But since it isn't, let's make sure the users/admins are aware about what is happening.
With respect to the InterruptedExceptions, we can log them as debug, but ignoring them completely wouldn't be nice.
Cheers,
Wilder
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack 8656 adding messages to empty ...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/637#issuecomment-126233316
Changes LGTM, since we are creating a separate bug for correctly handling log levels.
@DaanHoogland I am not saying to remove the interrupted exceptions, just that logging messages for them may not be needed. But I agree with @wilderrodrigues that log level for these may be changed to DEBUG
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack 8656 adding messages to empty ...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/637#issuecomment-126238495
So @koushik-das Do you propose the folowing?
- merge this for 4.6
- make a critical ticket for 4.7 to asses log levels
note that I am not completely done yet and will have addressed all but the hyperv console before the end of the weekend hopefully.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack 8656 adding messages to empty ...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/637#issuecomment-126202911
@koushik-das, Ignoring interrupted exceptions means that you do not allow the user or other threads to interrupt you. To me that seems serious. I did not in any case see comment about why it was done so I rigorously added the messages.
As for level; On one hand you are right @wilderrodrigues , as some errors that have been ignored in the past have been problems. On the other hand, someone did think it was safe to ignore those errors. The real job is to asses all the messages on level but I regard that out of scope for this PR. Maybe some of them should be WARN or even ERROR and others merely DEBUG or TRACE. I put the tag [ignored] there so we know we must considder them for either proper handling or another level of logging.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack 8656 adding messages to empty ...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/637#issuecomment-126185823
Since all these "ignore" messages are logged at INFO level, it needs to be ensured that logs are not getting filled up too soon. Also I see lot of instances for InterruptedException, I think they can they be excluded.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Cloudstack 8656
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/637#issuecomment-126094297
@wilderrodrigues @wido @bhaisaab @rajesh-battala @koushik-das @kishankavala @K0zka
I am taking this by exception type now so this is going to give conflicts pretty soon. I will continue on file by file basis to reduce the chances for that.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---