You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by John McDonnell <no...@github.com> on 2015/08/17 22:33:45 UTC
[jclouds] Adding a size parameter to the cloudstack usage record
(#845)
While working with cloudstack and jclouds, we noticed that the volume usage record was missing a size parameter that's in the cloudstack docs, but missing from jclouds. Filed a jira ticket for this, JCLOUDS-991.
Basically just added in a size, and updated the unit test.
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds/pull/845
-- Commit Summary --
* As in JCLOUDS-991 it appears size is missing as a UsageResponse parameter, particularily for Volume usages. This commit adds it into the UsageRecord class.
-- File Changes --
M apis/cloudstack/src/main/java/org/jclouds/cloudstack/domain/UsageRecord.java (29)
M apis/cloudstack/src/test/java/org/jclouds/cloudstack/parse/ListUsageRecordsResponseTest.java (3)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/845.patch
https://github.com/jclouds/jclouds/pull/845.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/845
Re: [jclouds] Adding a size parameter to the cloudstack usage record
(#845)
Posted by John McDonnell <no...@github.com>.
Hi Ignasi,
Sorry, been away for a few days.
I don’t have access to that code that the moment, where the NPE was being thrown, so Im not able to investigate that further. Looking at the cloudstack docs http://cloudstack-administration.readthedocs.org/en/latest/usage.html#usage-record-format <http://cloudstack-administration.readthedocs.org/en/latest/usage.html#usage-record-format> size isn’t provided in every usage record, much like virtualMachineId isn’t.
So I suppose then that what you said makes sense and we should go back to the Long type with @Nullable…
I can make this change later on and maybe have a look at the unit test thats there, and see if I can work out what my issue was back then...
John
> On 18 Sep 2015, at 11:02, Ignasi Barrera <no...@github.com> wrote:
>
> @mcdonnell-john <https://github.com/mcdonnell-john> ping?
>
> —
> Reply to this email directly or view it on GitHub <https://github.com/jclouds/jclouds/pull/845#issuecomment-141409058>.
>
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/845#issuecomment-142007489
Re: [jclouds] Adding a size parameter to the cloudstack usage record
(#845)
Posted by John McDonnell <no...@github.com>.
Thank you. I will do this tomorrow.
On 7 October 2015 at 21:27, Ignasi Barrera <no...@github.com> wrote:
> @mcdonnell-john <https://github.com/mcdonnell-john> I've just squashed
> the previous commits and pushed them to master
> <http://git-wip-us.apache.org/repos/asf/jclouds/commit/c73b82b5> and 1.9.x
> <http://git-wip-us.apache.org/repos/asf/jclouds/commit/9cd78bfc>. The
> commits have already been mirrored to GitHub, so if you start a clean
> branch from master you should be able to start with the changes in this PR
> and in a clean state.
>
> The right changes are merged now, so I'm closing this PR. Feel free to
> raise a new PR to add more fixes starting from the latest master. Does this
> work for you?
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/jclouds/jclouds/pull/845#issuecomment-146318152>.
>
--
John
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/845#issuecomment-146320131
Re: [jclouds] Adding a size parameter to the cloudstack usage record
(#845)
Posted by John McDonnell <no...@github.com>.
thanks @nacx for the comment. you're right it can be null, so I have made it a Long.
Any idea on the timeframe for your next release? (2.0.X is fine for this now)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/845#issuecomment-133204733
Re: [jclouds] Adding a size parameter to the cloudstack usage record
(#845)
Posted by Ignasi Barrera <no...@github.com>.
Thanks @mcdonnell-john! I've just added one small comment.
Regarding the backport to 1.7.x, we can cherry-pick the commit, but there are no plans for another 1.7 bugfix release.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/845#issuecomment-133200731
Re: [jclouds] Adding a size parameter to the cloudstack usage record
(#845)
Posted by John McDonnell <no...@github.com>.
@nacx I think I have made an error here somewhere along the lines.
I filed a new JCLOUDS defect earlier, 1012, and attempted to update my fork of jclouds and then make the changes for this, but it appears as though my commit has become part of this pull request, when I think it should almost definitely be as another.
Any advise on the best way to fix what I have done?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/845#issuecomment-146316621
Re: [jclouds] Adding a size parameter to the cloudstack usage record
(#845)
Posted by Ignasi Barrera <no...@github.com>.
@mcdonnell-john I've just squashed the previous commits and pushed them to [master](http://git-wip-us.apache.org/repos/asf/jclouds/commit/c73b82b5) and [1.9.x](http://git-wip-us.apache.org/repos/asf/jclouds/commit/9cd78bfc). The commits have already been mirrored to GitHub, so if you start a clean branch from master you should be able to start with the changes in this PR and in a clean state.
The right changes are merged now, so I'm closing this PR. Feel free to raise a new PR to add more fixes starting from the latest master. Does this work for you?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/845#issuecomment-146318152
Re: [jclouds] Adding a size parameter to the cloudstack usage record
(#845)
Posted by John McDonnell <no...@github.com>.
@nacx
Actually I think my first change was still wrong, as I'm witnessing a NPE locally when working with usage that doesn't include a size parameter.
I've gone back to the primitive and remove the @Nullable annotation now.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/845#issuecomment-134261494
Re: [jclouds] Adding a size parameter to the cloudstack usage record
(#845)
Posted by Ignasi Barrera <no...@github.com>.
It appears that there is a general re-formatting of the changed class. Could you revert it so the PR only contains your actual change?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/845#issuecomment-136517597
Re: [jclouds] Adding a size parameter to the cloudstack usage record
(#845)
Posted by Ignasi Barrera <no...@github.com>.
Closed #845.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/845#event-429582149
Re: [jclouds] Adding a size parameter to the cloudstack usage record
(#845)
Posted by Ignasi Barrera <no...@github.com>.
Thx for the fix! Having a look at the [CloudStack docs](http://cloudstack-administration.readthedocs.org/en/latest/usage.html#disk-volume-usage-record-format), it seems that the *size* field is not returned in all usage types. I'm not very familiar with the CloudStack provider, but if I'm not wrong, the current implementation reuses the UsageRecord entity for all types, so the *size* field could actually be null in some cases. In that case, we should keep the Long object with the `@Nullable` annotation.
Can you share the issues you experienced with that approach, to see if we can fix it?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/845#issuecomment-136520689
Re: [jclouds] Adding a size parameter to the cloudstack usage record
(#845)
Posted by Ignasi Barrera <no...@github.com>.
@mcdonnell-john ping?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/845#issuecomment-141409058
Re: [jclouds] Adding a size parameter to the cloudstack usage record
(#845)
Posted by John McDonnell <no...@github.com>.
Is it possible for this to be back merged to 1.7? or will I need to do another pull request?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/845#issuecomment-132101262
Re: [jclouds] Adding a size parameter to the cloudstack usage record
(#845)
Posted by John McDonnell <no...@github.com>.
My bad, I made the last change on my work computer which has specific formatting rules. should be fixed now.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/845#issuecomment-136518985
Re: [jclouds] Adding a size parameter to the cloudstack usage record
(#845)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -330,19 +340,21 @@ protected ConcreteBuilder self() {
> private final boolean isSourceNAT;
> private final double rawUsageHours;
> private final String usage;
> + private final long size;
Is this parameter returned *always*? I see it declared as `@Nullable`. If it can be missing, then declare the variable as a `Long` object (primitives can't be null and NPEs will appear); otherwise remove the nullable annotation.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/845/files#r37588447
Re: [jclouds] Adding a size parameter to the cloudstack usage record
(#845)
Posted by Ignasi Barrera <no...@github.com>.
> I can make this change later on and maybe have a look at the unit test thats there, and see if I can work out what my issue was back then...
With this change we should be aligned with the API docs, so I'd say go ahead :)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/845#issuecomment-142107755
Re: [jclouds] Adding a size parameter to the cloudstack usage record
(#845)
Posted by Ignasi Barrera <no...@github.com>.
Thx! The change lgtm. Mind squashing the commits into a single one so I can cleanly merge the PR?
>Any idea on the timeframe for your next release? (2.0.X is fine for this now)
Not yet. This has not been officially discussed yet, but there are some issues and features we know we want in 2.0 so that will mostly determine the possible release timeframe. In any case, feel free to start a discussion in the [dev@ mailing list](http://jclouds.apache.org/community/) to make sure things start moving :)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/845#issuecomment-133205506