You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by agneya2001 <gi...@git.apache.org> on 2015/12/28 12:20:46 UTC
[GitHub] cloudstack pull request: Quota: findbug fixes
GitHub user agneya2001 opened a pull request:
https://github.com/apache/cloudstack/pull/1289
Quota: findbug fixes
Findbug fixes for cloud-framework-quota and cloud-plugin-database-quota.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/shapeblue/cloudstack master-quotafb
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/1289.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 #1289
----
commit 625abb80aa590cfc7e6cfefe6f0bf55c95ac679f
Author: Abhinandan Prateek <ag...@gmail.com>
Date: 2015-12-23T04:42:11Z
Quota: findbug fixes
----
---
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: Quota: findbug fixes
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1289#issuecomment-167554252
Based on the code LGTM, tests running
---
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: Quota: findbug fixes
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1289#issuecomment-172558281
again code LGTM. Let's quickly run it 3 out of 5 findbugs issues will be fixed in this.
---
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: Quota: findbug fixes
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1289#issuecomment-172597438
here is my original request to look at them:
http://markmail.org/message/fqv252o3p7hmzwxz
---
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: Quota: findbug fixes
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1289#issuecomment-172563054
What needs to be done to fix them all?
---
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: Quota: findbug fixes
Posted by agneya2001 <gi...@git.apache.org>.
Github user agneya2001 commented on the pull request:
https://github.com/apache/cloudstack/pull/1289#issuecomment-172418090
@remibergsma @bhaisaab made review changes
---
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: Quota: findbug fixes
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/1289
---
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: Quota: findbug fixes
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/1289#issuecomment-175479331
LGTM @agneya2001 master seems to be unfrozen you'll need one more review to merge it yourself
---
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: Quota: findbug fixes
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1289#discussion_r48474787
--- Diff: framework/quota/src/org/apache/cloudstack/quota/vo/ServiceOfferingVO.java ---
@@ -312,7 +271,7 @@ public String getDeploymentPlanner() {
public String getDetail(String name) {
assert (details != null) : "Did you forget to load the details?";
--- End diff --
I would have solved this one by removing the assert. Do we even run any tests with asserts enabled? (no :-1: intended!)
---
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: Quota: findbug fixes
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1289#discussion_r49295533
--- Diff: framework/quota/src/org/apache/cloudstack/quota/vo/ServiceOfferingVO.java ---
@@ -310,14 +269,12 @@ public String getDeploymentPlanner() {
}
public String getDetail(String name) {
- assert (details != null) : "Did you forget to load the details?";
-
- return details != null ? details.get(name) : null;
+ if (details == null) throw new IllegalStateException("Cannot set value as details is null");
+ return details.get(name);
}
public void setDetail(String name, String value) {
- assert (details != null) : "Did you forget to load the details?";
-
+ if (details == null) throw new IllegalStateException("Cannot set value as details is null");
--- End diff --
I think that would work (returning an empty map) though we need to check the history as to why we never did that earlier. For now, the fix LGTM as it's better than the assert. We can take the hashmap fix separately.
---
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: Quota: findbug fixes
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1289#issuecomment-169753995
Would you mind changing the comments over attributes “details” and “isDynamic” to proper java doc style? Additionally, what about changing them to private?
---
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: Quota: findbug fixes
Posted by agneya2001 <gi...@git.apache.org>.
Github user agneya2001 commented on the pull request:
https://github.com/apache/cloudstack/pull/1289#issuecomment-175472278
This patch cleans up any find bug issues in cloud-framework-quota and cloud-plugin-database-quota, verifiy output below. I think we should not delay this @remibergsma @DaanHoogland @bhaisaab @rafaelweingartner
mvn -P enablefindbugs -pl :cloud-framework-quota verify
...
[INFO] --- maven-compiler-plugin:3.2:compile (default-compile) @ cloud-framework-quota ---
[INFO] Nothing to compile - all classes are up to date
[INFO]
[INFO] >>> findbugs-maven-plugin:3.0.1:check (cloudstack-findbugs) > :findbugs @ cloud-framework-quota >>>
[INFO]
[INFO] --- findbugs-maven-plugin:3.0.1:findbugs (findbugs) @ cloud-framework-quota ---
[INFO] Fork Value is true
[INFO] Done FindBugs Analysis....
[INFO]
[INFO] <<< findbugs-maven-plugin:3.0.1:check (cloudstack-findbugs) < :findbugs @ cloud-framework-quota <<<
[INFO]
[INFO] --- findbugs-maven-plugin:3.0.1:check (cloudstack-findbugs) @ cloud-framework-quota ---
[INFO] BugInstance size is 0
[INFO] Error size is 0
[INFO] No errors/warnings found
[INFO]
mvn -P enablefindbugs -pl :cloud-plugin-database-quota verify
..
[INFO] >>> findbugs-maven-plugin:3.0.1:check (cloudstack-findbugs) > :findbugs @ cloud-plugin-database-quota >>>
[INFO]
[INFO] --- findbugs-maven-plugin:3.0.1:findbugs (findbugs) @ cloud-plugin-database-quota ---
[INFO] Fork Value is true
[INFO] Done FindBugs Analysis....
[INFO]
[INFO] <<< findbugs-maven-plugin:3.0.1:check (cloudstack-findbugs) < :findbugs @ cloud-plugin-database-quota <<<
[INFO]
[INFO] --- findbugs-maven-plugin:3.0.1:check (cloudstack-findbugs) @ cloud-plugin-database-quota ---
[INFO] BugInstance size is 0
[INFO] Error size is 0
[INFO] No errors/warnings found
---
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: Quota: findbug fixes
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/1289#issuecomment-167625390
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: Quota: findbug fixes
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/1289#issuecomment-170455889
@agneya2001 can you help fix the issues, before the freeze. Thanks.
---
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: Quota: findbug fixes
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1289#issuecomment-172197923
Ping @agneya2001 let's get this resolved soon :-) All those build failure mails hurt my eyes..
---
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: Quota: findbug fixes
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1289#discussion_r48476072
--- Diff: framework/quota/src/org/apache/cloudstack/quota/vo/ServiceOfferingVO.java ---
@@ -310,14 +269,12 @@ public String getDeploymentPlanner() {
}
public String getDetail(String name) {
- assert (details != null) : "Did you forget to load the details?";
-
- return details != null ? details.get(name) : null;
+ if (details == null) throw new IllegalStateException("Cannot set value as details is null");
+ return details.get(name);
}
public void setDetail(String name, String value) {
- assert (details != null) : "Did you forget to load the details?";
-
+ if (details == null) throw new IllegalStateException("Cannot set value as details is null");
--- End diff --
good solution :+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: Quota: findbug fixes
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1289#discussion_r49316008
--- Diff: framework/quota/src/org/apache/cloudstack/quota/vo/ServiceOfferingVO.java ---
@@ -310,14 +269,12 @@ public String getDeploymentPlanner() {
}
public String getDetail(String name) {
- assert (details != null) : "Did you forget to load the details?";
-
- return details != null ? details.get(name) : null;
+ if (details == null) throw new IllegalStateException("Cannot set value as details is null");
+ return details.get(name);
}
public void setDetail(String name, String value) {
- assert (details != null) : "Did you forget to load the details?";
-
+ if (details == null) throw new IllegalStateException("Cannot set value as details is null");
--- End diff --
We can check that, but if we will let it as it is.
I think it should have a test case written to check that behavior.
---
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: Quota: findbug fixes
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1289#discussion_r49101734
--- Diff: framework/quota/src/org/apache/cloudstack/quota/vo/ServiceOfferingVO.java ---
@@ -310,14 +269,12 @@ public String getDeploymentPlanner() {
}
public String getDetail(String name) {
- assert (details != null) : "Did you forget to load the details?";
-
- return details != null ? details.get(name) : null;
+ if (details == null) throw new IllegalStateException("Cannot set value as details is null");
+ return details.get(name);
}
public void setDetail(String name, String value) {
- assert (details != null) : "Did you forget to load the details?";
-
+ if (details == null) throw new IllegalStateException("Cannot set value as details is null");
--- End diff --
Do you really like this kind of solution? A POJO with some kind of verification does not seem like the best thing to have.
Why don’t we initialize the as an empty map? This way it would be always ready for use, and we would not need that verification. We could change line 81 to Map<String, String> details = new HashMap<String, String> ();
---
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: Quota: findbug fixes
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1289#discussion_r49101315
--- Diff: framework/quota/src/org/apache/cloudstack/quota/vo/ServiceOfferingVO.java ---
@@ -310,14 +269,12 @@ public String getDeploymentPlanner() {
}
public String getDetail(String name) {
- assert (details != null) : "Did you forget to load the details?";
-
- return details != null ? details.get(name) : null;
+ if (details == null) throw new IllegalStateException("Cannot set value as details is null");
+ return details.get(name);
}
public void setDetail(String name, String value) {
--- End diff --
what about a change in this method name?
I think "addDetail" would be better.
---
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: Quota: findbug fixes
Posted by agneya2001 <gi...@git.apache.org>.
Github user agneya2001 commented on the pull request:
https://github.com/apache/cloudstack/pull/1289#issuecomment-175500735
merging LGTM from @DaanHoogland and @bhaisaab
---
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: Quota: findbug fixes
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1289#issuecomment-168489999
Ping @bhaisaab @agneya2001 the quota plugin unit tests are failing in `4.7` and `master` branches:
```
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 02:44 min (Wall Clock)
[INFO] Finished at: 2016-01-03T09:51:53+01:00
[INFO] Final Memory: 78M/1371M
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:2.18.1:test (default-test) on project cloud-framework-quota: There are test failures.
[ERROR]
```
Due to:
```
Tests run: 6, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.054 sec
<<< FAILURE! - in org.apache.cloudstack.quota.QuotaStatementTest
testSendStatement(org.apache.cloudstack.quota.QuotaStatementTest) Time
elapsed: 0.011 sec <<< FAILURE!
org.mockito.exceptions.verification.WantedButNotInvoked:
Wanted but not invoked:
alertManager.sendQuotaAlert(
org.apache.cloudstack.quota.QuotaAlertManagerImpl$DeferredQuotaEmail@6e9d6ff7
);
-> at
org.apache.cloudstack.quota.QuotaStatementTest.testSendStatement(QuotaStatementTest.java:251)
Actually, there were zero interactions with this mock.
at
org.apache.cloudstack.quota.QuotaStatementTest.testSendStatement(QuotaStatementTest.java:251)
Results :
Failed tests:
QuotaStatementTest.testSendStatement:251
Wanted but not invoked:
alertManager.sendQuotaAlert(
org.apache.cloudstack.quota.QuotaAlertManagerImpl$DeferredQuotaEmail@6e9d6ff7
);
-> at
org.apache.cloudstack.quota.QuotaStatementTest.testSendStatement(QuotaStatementTest.java:251)
```
Please fix against `4.7` branch after which we will forward merge to `master`. Thanks!
---
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: Quota: findbug fixes
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1289#issuecomment-172594983
@remibergsma I would have to dive into my original investigation again. I am not sure who or where the others where created anymore. lat me markmail that for you.
---
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.
---