Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/850
---
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
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/850#issuecomment-149862804
Sure, I think that a more specific âthrowsâ clause has to be used.
Throws exception, was just an expression.
---
If your project is set up for it, y
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/850#issuecomment-149862051
not "throws Exception" but a more specific throws. In this case the
exceptions are silently ignored as part of a happy flow. I don't like that but
I'm not goin
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/850#issuecomment-149861633
If an exception occurs, is the test still going to pass?
If that is not the case, I would use "throws Exception" on each one of
those tests.
---
If y
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/850#issuecomment-149818277
the reason for ignoring should be made more explicit in one way or the
other. I'm not hellbound on logging but I see no problem in it. A caught and
ignored exce
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/850#issuecomment-149744151
Do you guys really think that is a good idea to log exceptions in a test
case?
If the case is just to ignore them, it could be done without logging.
Github user ustcweizhou commented on the pull request:
https://github.com/apache/cloudstack/pull/850#issuecomment-149162987
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
e
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/850#issuecomment-144384074
@DaanHoogland @borisroman The test needs to invoke a protected method from
a class and so is done using reflection. The test case already asserts for the
method
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/850#issuecomment-144363576
@borisroman the expeted attribute is not there and as I understand with
reason. The exceptions are part of the contract and need to be handled by the
client. Th
Github user borisroman commented on the pull request:
https://github.com/apache/cloudstack/pull/850#issuecomment-141662154
@DaanHoogland Why log when an exception is thrown? In a test when an
exception is thrown it should either fail, or when @Test(expected =
--exception.class--) is d
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/850#issuecomment-141620583
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
e
GitHub user DaanHoogland opened a pull request:
https://github.com/apache/cloudstack/pull/850
CLOUDSTACK-8656: tests ignoring exceptions
final few ignored exceptions supplied with log messages.
You can merge this pull request into a Git repository by running:
$ git pull https:/
Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/654
---
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
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/654#issuecomment-131214770
@bhaisaab Indeed, let's fix forward when needed :-) There's always a risk,
but I'm happy to see this work being done :+1:
---
If your project is set up for it,
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/654#issuecomment-131212079
@DaanHoogland I agree, we should only squash if there is no value to have
the commits separated. I usually ask for squashing when feedback was processed
in a sep
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/654#issuecomment-131181130
You know I am a great advocate of not squashing, right. All commits are
atomic. Yes there is a lot because the work has also been scattered and a lot.
---
If y
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/654#issuecomment-131179496
Another way of fixing a regression could be to simply fix the issue,
instead of reverting at all.
---
If your project is set up for it, you can reply to this email
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/654#issuecomment-131178239
All kinds of regressions may be introduced. This is (one of the reasons)
why I will not squash. for a single regression we only have to revert a single
commit,
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/654#issuecomment-131175032
look alright, but could it introduce any regressions in the upgrade path?
Also, I would love to see a single squashed patched with the changes given ~30
patches for
Github user wido commented on the pull request:
https://github.com/apache/cloudstack/pull/654#issuecomment-131164822
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
Github user mike-tutkowski commented on the pull request:
https://github.com/apache/cloudstack/pull/654#issuecomment-129990097
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 featur
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/654#discussion_r36774764
--- Diff:
services/console-proxy/server/src/com/cloud/consoleproxy/vnc/packet/server/RawRect.java
---
@@ -50,26 +52,27 @@ public void paint(Buffer
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/654#discussion_r36167090
--- Diff:
vmware-base/src/com/cloud/hypervisor/vmware/mo/SnapshotDescriptor.java ---
@@ -166,12 +168,7 @@ public void removeDiskReferenceFromSnapshot
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/654#discussion_r36166742
--- Diff:
vmware-base/src/com/cloud/hypervisor/vmware/mo/SnapshotDescriptor.java ---
@@ -166,12 +168,7 @@ public void removeDiskReferenceFromSnapshot(
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/654#discussion_r36166324
--- Diff: utils/src/com/cloud/utils/AutoCloseableUtil.java ---
@@ -0,0 +1,20 @@
+package com.cloud.utils;
--- End diff --
License?
-
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/654#discussion_r36141245
--- Diff: utils/src/com/cloud/utils/AutoCloseableUtil.java ---
@@ -0,0 +1,20 @@
+package com.cloud.utils;
+
+import org.apache.log4j.Log
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/654#discussion_r36141156
--- Diff: framework/db/src/com/cloud/utils/db/DbUtil.java ---
@@ -43,8 +43,10 @@
import org.apache.log4j.Logger;
+import static
GitHub user DaanHoogland opened a pull request:
https://github.com/apache/cloudstack/pull/654
Cloudstack 8656: do away with more silently ignoring exceptions
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/DaanHoogland/cloudstack
Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/649
---
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
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/649#issuecomment-127398255
two lgtm and succesful travis, merge gives no conflicts but will do a local
build before push anyway
---
If your project is set up for it, you can reply to thi
Github user mike-tutkowski commented on the pull request:
https://github.com/apache/cloudstack/pull/649#issuecomment-127359986
I just had a comment on 87ae150.
---
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 proje
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/649#issuecomment-127357042
@mike-tutkowski sorry didn't notice your reply, I added a view more. Can
you have a look?
---
If your project is set up for it, you can reply to this email and
Yes
On Monday, August 3, 2015, DaanHoogland wrote:
> Github user DaanHoogland commented on the pull request:
>
> https://github.com/apache/cloudstack/pull/649#issuecomment-127284140
>
> @mike-tutkowski are you alright with this now (merge-level allright;)?
>
>
> ---
> If your project is
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/649#issuecomment-127284140
@mike-tutkowski are you alright with this now (merge-level allright;)?
---
If your project is set up for it, you can reply to this email and have your
reply app
Github user wilderrodrigues commented on the pull request:
https://github.com/apache/cloudstack/pull/649#issuecomment-127174572
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
Github user mike-tutkowski commented on the pull request:
https://github.com/apache/cloudstack/pull/649#issuecomment-127112655
Aside from a few comments on 04e9083c3199effa295e47bc617b9b406adb6109, LGTM.
---
If your project is set up for it, you can reply to this email and have your
r
GitHub user DaanHoogland opened a pull request:
https://github.com/apache/cloudstack/pull/649
Cloudstack 8656: do away with silently ignoring exceptions
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/DaanHoogland/cloudstack CLOU
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
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 p
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 ha
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 d
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
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 h
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
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
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 nex
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 an
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
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 h
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
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
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 CL
52 matches
Mail list logo
