You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Nate Gordon <na...@appcore.com> on 2014/05/02 00:35:11 UTC
Re: Review Request 20557: CLOUDSTACK-6472 listUsageRecords generates
NPEs for expunging instances
Sorry for being late to the discussion, I believe this patch doesn't fully
do what is actually expected. This patch does prevent the NPE, but doesn't
prevent the reason why the NPE was happening in the first place. It appears
that in commit f5e5b39, all the instances of
find{something}ByIdIncludingRemoved(id) where changed to
find{something}ById(id). This was 9 months ago, and since then it was
changed again to _entityMgr.findById({something}.class, id). So the reason
for the NPE is actually that we aren't querying for removed entries, but
those entries still exist, and used to be returned by the listUsageRecords
api. This patch mostly just prevents the UUID field from being populated in
those instances, which are relied upon by myself and surely others to
correlate records together.
I'll try to get a new patch put together tonight that addresses that,
unless there is some other commentary as to why findByIdIncludingRemoved
was not included in the EntityManager interface, but is still present in
EntityManagerImpl.
Having said that, I was just commenting to someone today that it would be
nice if there was some null checking done in this process and am glad that
it exists now. We have instances from time to time where something just
gets flat out removed from the db and that tends to break usage which
requires some unfortunate debugging effort.
Thanks,
-Nate
On Fri, Apr 25, 2014 at 4:01 PM, Sebastien Goasguen <ru...@gmail.com>wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20557/#review41524
> -----------------------------------------------------------
>
> Ship it!
>
>
> Applied to 4.3 with commit:
> 08997a9ba37d939dc6e546c632daf93b2b04e825
>
> I re-wrote the patch and committed to master as well with:
> 744e2a54e8b05d8136382664d8e5b9e3649fe88e
>
> Thanks for the patch, please make sure to tell us if there is more issue
> with this.
>
> You can mark the review as submitted.
>
>
> - Sebastien Goasguen
>
>
> On April 23, 2014, 12:47 p.m., Pierre-Yves Ritschard wrote:
> >
> > -----------------------------------------------------------
> > This is an automatically generated e-mail. To reply, visit:
> > https://reviews.apache.org/r/20557/
> > -----------------------------------------------------------
> >
> > (Updated April 23, 2014, 12:47 p.m.)
> >
> >
> > Review request for cloudstack.
> >
> >
> > Bugs: CLOUDSTACK-6472
> > https://issues.apache.org/jira/browse/CLOUDSTACK-6472
> >
> >
> > Repository: cloudstack-git
> >
> >
> > Description
> > -------
> >
> > This is a review request for CLOUDSTACK-6472 "listUsageRecords generates
> NPEs for expunging instances"
> >
> > The patch is against the 4.3 branch
> >
> >
> > Diffs
> > -----
> >
> > server/src/com/cloud/api/ApiResponseHelper.java e543d1c
> >
> > Diff: https://reviews.apache.org/r/20557/diff/
> >
> >
> > Testing
> > -------
> >
> >
> > Thanks,
> >
> > Pierre-Yves Ritschard
> >
> >
>
>
--
*Nate Gordon*Director of Technology | Appcore - the business of cloud
computing®
Office +1.800.735.7104 | Direct +1.515.612.7787
nate.gordon@appcore.com | www.appcore.com
----------------------------------------------------------------------
The information in this message is intended for the named recipients only.
It may contain information that is privileged, confidential or otherwise
protected from disclosure. If you are not the intended recipient, you are
hereby notified that any disclosure, copying, distribution, or the taking
of any action in reliance on the contents of this message is strictly
prohibited. If you have received this e-mail in error, do not print it or
disseminate it or its contents. In such event, please notify the sender by
return e-mail and delete the e-mail file immediately thereafter. Thank you.
Re: Review Request 20557: CLOUDSTACK-6472 listUsageRecords generates
NPEs for expunging instances
Posted by Sam Schmit <sa...@appcore.com>.
Olivier,
I've submitted the following review for 4.3:
https://reviews.apache.org/r/21088/
It does, in fact, work to add the findByIdIncludingRemoved to the
EntityManager. Thus, this should merge into 4.3 with no problems.
Thanks,
Sam
On Mon, May 5, 2014 at 10:23 AM, Sam Schmit <sa...@appcore.com> wrote:
> Olivier,
>
> I'm still checking into it, but it may work - as you said, it should be a
> supported method. I've already made the change on my dev machine, and I'm
> getting set up with a development cloud to test it out. I should know
> later today if it will work for 4.3.
>
> Sam
>
>
> On Mon, May 5, 2014 at 10:04 AM, Olivier Lemasle <
> olivier.lemasle@apalia.net> wrote:
>
>> Hi Sam,
>>
>> I was going to suggest the same patch than you (with
>> findByIdIncludingRemoved), and I hit the same issue: no method
>> findByIdIncludingRemoved in com.cloud.utils.db.EntityManager.
>> But I see that the implementing class com.cloud.dao.EntityManagerImpl
>> still
>> has findByIdIncludingRemoved(). Do you know why it was removed from the
>> interface in f5e5b39c9bc8d?
>>
>> --
>> Olivier
>>
>>
>> On Mon, May 5, 2014 at 3:45 PM, Sam Schmit <sa...@appcore.com>
>> wrote:
>>
>> > Sebastien,
>> >
>> > I took a look at the 4.3 code, and I was mistaken - it will not commit
>> > cleanly. The EntityManager does not have the findIdIncludingRemoved
>> > function added yet. I'll have to spend more time looking into how to
>> get
>> > this into 4.3 cleanly.
>> >
>> > Sam
>> >
>> >
>> >
>> > On Fri, May 2, 2014 at 3:57 PM, Sebastien Goasguen <ru...@gmail.com>
>> > wrote:
>> >
>> > >
>> > > On May 2, 2014, at 2:17 PM, Sam Schmit <sa...@appcore.com>
>> wrote:
>> > >
>> > > > All,
>> > > >
>> > > > I have submitted a bug fix patch for this issue:
>> > > >
>> > > > https://reviews.apache.org/r/21015/diff/
>> > > >
>> > > > If approved, this fix should also find its way into the 4.3 and 4.4
>> > > builds. It is fairly critical for getting usage data out of
>> cloudstack.
>> > > >
>> > >
>> > > Sam, thanks a lot for the patch. Since You and Pyr actually use this
>> in
>> > > prod, I will rely on your for testing :)
>> > >
>> > > I applied your patch to master and 4.4-forward.
>> > >
>> > > I need a clean patch for 4.3
>> > >
>> > > If you could check 4.4-forward and tell me if it's ok then I will
>> request
>> > > a cherry-pick shortly (to make the first RC).
>> > >
>> > >
>> > > > Sam Schmit
>> > > >
>> > > >
>> > > > On Thu, May 1, 2014 at 5:35 PM, Nate Gordon <
>> nate.gordon@appcore.com>
>> > > wrote:
>> > > > Sorry for being late to the discussion, I believe this patch doesn't
>> > > fully
>> > > > do what is actually expected. This patch does prevent the NPE, but
>> > > doesn't
>> > > > prevent the reason why the NPE was happening in the first place. It
>> > > appears
>> > > > that in commit f5e5b39, all the instances of
>> > > > find{something}ByIdIncludingRemoved(id) where changed to
>> > > > find{something}ById(id). This was 9 months ago, and since then it
>> was
>> > > > changed again to _entityMgr.findById({something}.class, id). So the
>> > > reason
>> > > > for the NPE is actually that we aren't querying for removed entries,
>> > but
>> > > > those entries still exist, and used to be returned by the
>> > > listUsageRecords
>> > > > api. This patch mostly just prevents the UUID field from being
>> > populated
>> > > in
>> > > > those instances, which are relied upon by myself and surely others
>> to
>> > > > correlate records together.
>> > > >
>> > > > I'll try to get a new patch put together tonight that addresses
>> that,
>> > > > unless there is some other commentary as to why
>> > findByIdIncludingRemoved
>> > > > was not included in the EntityManager interface, but is still
>> present
>> > in
>> > > > EntityManagerImpl.
>> > > >
>> > > > Having said that, I was just commenting to someone today that it
>> would
>> > be
>> > > > nice if there was some null checking done in this process and am
>> glad
>> > > that
>> > > > it exists now. We have instances from time to time where something
>> just
>> > > > gets flat out removed from the db and that tends to break usage
>> which
>> > > > requires some unfortunate debugging effort.
>> > > >
>> > > > Thanks,
>> > > >
>> > > > -Nate
>> > > >
>> > > >
>> > > > On Fri, Apr 25, 2014 at 4:01 PM, Sebastien Goasguen <
>> runseb@gmail.com
>> > > >wrote:
>> > > >
>> > > > >
>> > > > > -----------------------------------------------------------
>> > > > > This is an automatically generated e-mail. To reply, visit:
>> > > > > https://reviews.apache.org/r/20557/#review41524
>> > > > > -----------------------------------------------------------
>> > > > >
>> > > > > Ship it!
>> > > > >
>> > > > >
>> > > > > Applied to 4.3 with commit:
>> > > > > 08997a9ba37d939dc6e546c632daf93b2b04e825
>> > > > >
>> > > > > I re-wrote the patch and committed to master as well with:
>> > > > > 744e2a54e8b05d8136382664d8e5b9e3649fe88e
>> > > > >
>> > > > > Thanks for the patch, please make sure to tell us if there is more
>> > > issue
>> > > > > with this.
>> > > > >
>> > > > > You can mark the review as submitted.
>> > > > >
>> > > > >
>> > > > > - Sebastien Goasguen
>> > > > >
>> > > > >
>> > > > > On April 23, 2014, 12:47 p.m., Pierre-Yves Ritschard wrote:
>> > > > > >
>> > > > > > -----------------------------------------------------------
>> > > > > > This is an automatically generated e-mail. To reply, visit:
>> > > > > > https://reviews.apache.org/r/20557/
>> > > > > > -----------------------------------------------------------
>> > > > > >
>> > > > > > (Updated April 23, 2014, 12:47 p.m.)
>> > > > > >
>> > > > > >
>> > > > > > Review request for cloudstack.
>> > > > > >
>> > > > > >
>> > > > > > Bugs: CLOUDSTACK-6472
>> > > > > > https://issues.apache.org/jira/browse/CLOUDSTACK-6472
>> > > > > >
>> > > > > >
>> > > > > > Repository: cloudstack-git
>> > > > > >
>> > > > > >
>> > > > > > Description
>> > > > > > -------
>> > > > > >
>> > > > > > This is a review request for CLOUDSTACK-6472 "listUsageRecords
>> > > generates
>> > > > > NPEs for expunging instances"
>> > > > > >
>> > > > > > The patch is against the 4.3 branch
>> > > > > >
>> > > > > >
>> > > > > > Diffs
>> > > > > > -----
>> > > > > >
>> > > > > > server/src/com/cloud/api/ApiResponseHelper.java e543d1c
>> > > > > >
>> > > > > > Diff: https://reviews.apache.org/r/20557/diff/
>> > > > > >
>> > > > > >
>> > > > > > Testing
>> > > > > > -------
>> > > > > >
>> > > > > >
>> > > > > > Thanks,
>> > > > > >
>> > > > > > Pierre-Yves Ritschard
>> > > > > >
>> > > > > >
>> > > > >
>> > > > >
>> > > >
>> > > >
>> > > > --
>> > > >
>> > > >
>> > > > *Nate Gordon*Director of Technology | Appcore - the business of
>> cloud
>> > > > computing®
>> > > >
>> > > > Office +1.800.735.7104 | Direct +1.515.612.7787
>> > > > nate.gordon@appcore.com | www.appcore.com
>> > > >
>> > > >
>> ----------------------------------------------------------------------
>> > > >
>> > > > The information in this message is intended for the named recipients
>> > > only.
>> > > > It may contain information that is privileged, confidential or
>> > otherwise
>> > > > protected from disclosure. If you are not the intended recipient,
>> you
>> > are
>> > > > hereby notified that any disclosure, copying, distribution, or the
>> > taking
>> > > > of any action in reliance on the contents of this message is
>> strictly
>> > > > prohibited. If you have received this e-mail in error, do not print
>> it
>> > or
>> > > > disseminate it or its contents. In such event, please notify the
>> sender
>> > > by
>> > > > return e-mail and delete the e-mail file immediately thereafter.
>> Thank
>> > > you.
>> > > >
>> > >
>> > >
>> >
>>
>>
>>
>> --
>> Olivier Lemasle
>> Ingénieur Logiciel
>> *Apalia*™
>> Mobile: +33-611-69-12-11
>>
>> *http://www.apalia.net <http://www.apalia.net>
>> <ol...@apalia.net>olivier.lemasle@apalia.net
>> <ol...@apalia.net>*
>>
>
>
Re: Review Request 20557: CLOUDSTACK-6472 listUsageRecords generates
NPEs for expunging instances
Posted by Sam Schmit <sa...@appcore.com>.
Olivier,
I'm still checking into it, but it may work - as you said, it should be a
supported method. I've already made the change on my dev machine, and I'm
getting set up with a development cloud to test it out. I should know
later today if it will work for 4.3.
Sam
On Mon, May 5, 2014 at 10:04 AM, Olivier Lemasle <olivier.lemasle@apalia.net
> wrote:
> Hi Sam,
>
> I was going to suggest the same patch than you (with
> findByIdIncludingRemoved), and I hit the same issue: no method
> findByIdIncludingRemoved in com.cloud.utils.db.EntityManager.
> But I see that the implementing class com.cloud.dao.EntityManagerImpl still
> has findByIdIncludingRemoved(). Do you know why it was removed from the
> interface in f5e5b39c9bc8d?
>
> --
> Olivier
>
>
> On Mon, May 5, 2014 at 3:45 PM, Sam Schmit <sa...@appcore.com> wrote:
>
> > Sebastien,
> >
> > I took a look at the 4.3 code, and I was mistaken - it will not commit
> > cleanly. The EntityManager does not have the findIdIncludingRemoved
> > function added yet. I'll have to spend more time looking into how to get
> > this into 4.3 cleanly.
> >
> > Sam
> >
> >
> >
> > On Fri, May 2, 2014 at 3:57 PM, Sebastien Goasguen <ru...@gmail.com>
> > wrote:
> >
> > >
> > > On May 2, 2014, at 2:17 PM, Sam Schmit <sa...@appcore.com> wrote:
> > >
> > > > All,
> > > >
> > > > I have submitted a bug fix patch for this issue:
> > > >
> > > > https://reviews.apache.org/r/21015/diff/
> > > >
> > > > If approved, this fix should also find its way into the 4.3 and 4.4
> > > builds. It is fairly critical for getting usage data out of
> cloudstack.
> > > >
> > >
> > > Sam, thanks a lot for the patch. Since You and Pyr actually use this in
> > > prod, I will rely on your for testing :)
> > >
> > > I applied your patch to master and 4.4-forward.
> > >
> > > I need a clean patch for 4.3
> > >
> > > If you could check 4.4-forward and tell me if it's ok then I will
> request
> > > a cherry-pick shortly (to make the first RC).
> > >
> > >
> > > > Sam Schmit
> > > >
> > > >
> > > > On Thu, May 1, 2014 at 5:35 PM, Nate Gordon <nate.gordon@appcore.com
> >
> > > wrote:
> > > > Sorry for being late to the discussion, I believe this patch doesn't
> > > fully
> > > > do what is actually expected. This patch does prevent the NPE, but
> > > doesn't
> > > > prevent the reason why the NPE was happening in the first place. It
> > > appears
> > > > that in commit f5e5b39, all the instances of
> > > > find{something}ByIdIncludingRemoved(id) where changed to
> > > > find{something}ById(id). This was 9 months ago, and since then it was
> > > > changed again to _entityMgr.findById({something}.class, id). So the
> > > reason
> > > > for the NPE is actually that we aren't querying for removed entries,
> > but
> > > > those entries still exist, and used to be returned by the
> > > listUsageRecords
> > > > api. This patch mostly just prevents the UUID field from being
> > populated
> > > in
> > > > those instances, which are relied upon by myself and surely others to
> > > > correlate records together.
> > > >
> > > > I'll try to get a new patch put together tonight that addresses that,
> > > > unless there is some other commentary as to why
> > findByIdIncludingRemoved
> > > > was not included in the EntityManager interface, but is still present
> > in
> > > > EntityManagerImpl.
> > > >
> > > > Having said that, I was just commenting to someone today that it
> would
> > be
> > > > nice if there was some null checking done in this process and am glad
> > > that
> > > > it exists now. We have instances from time to time where something
> just
> > > > gets flat out removed from the db and that tends to break usage which
> > > > requires some unfortunate debugging effort.
> > > >
> > > > Thanks,
> > > >
> > > > -Nate
> > > >
> > > >
> > > > On Fri, Apr 25, 2014 at 4:01 PM, Sebastien Goasguen <
> runseb@gmail.com
> > > >wrote:
> > > >
> > > > >
> > > > > -----------------------------------------------------------
> > > > > This is an automatically generated e-mail. To reply, visit:
> > > > > https://reviews.apache.org/r/20557/#review41524
> > > > > -----------------------------------------------------------
> > > > >
> > > > > Ship it!
> > > > >
> > > > >
> > > > > Applied to 4.3 with commit:
> > > > > 08997a9ba37d939dc6e546c632daf93b2b04e825
> > > > >
> > > > > I re-wrote the patch and committed to master as well with:
> > > > > 744e2a54e8b05d8136382664d8e5b9e3649fe88e
> > > > >
> > > > > Thanks for the patch, please make sure to tell us if there is more
> > > issue
> > > > > with this.
> > > > >
> > > > > You can mark the review as submitted.
> > > > >
> > > > >
> > > > > - Sebastien Goasguen
> > > > >
> > > > >
> > > > > On April 23, 2014, 12:47 p.m., Pierre-Yves Ritschard wrote:
> > > > > >
> > > > > > -----------------------------------------------------------
> > > > > > This is an automatically generated e-mail. To reply, visit:
> > > > > > https://reviews.apache.org/r/20557/
> > > > > > -----------------------------------------------------------
> > > > > >
> > > > > > (Updated April 23, 2014, 12:47 p.m.)
> > > > > >
> > > > > >
> > > > > > Review request for cloudstack.
> > > > > >
> > > > > >
> > > > > > Bugs: CLOUDSTACK-6472
> > > > > > https://issues.apache.org/jira/browse/CLOUDSTACK-6472
> > > > > >
> > > > > >
> > > > > > Repository: cloudstack-git
> > > > > >
> > > > > >
> > > > > > Description
> > > > > > -------
> > > > > >
> > > > > > This is a review request for CLOUDSTACK-6472 "listUsageRecords
> > > generates
> > > > > NPEs for expunging instances"
> > > > > >
> > > > > > The patch is against the 4.3 branch
> > > > > >
> > > > > >
> > > > > > Diffs
> > > > > > -----
> > > > > >
> > > > > > server/src/com/cloud/api/ApiResponseHelper.java e543d1c
> > > > > >
> > > > > > Diff: https://reviews.apache.org/r/20557/diff/
> > > > > >
> > > > > >
> > > > > > Testing
> > > > > > -------
> > > > > >
> > > > > >
> > > > > > Thanks,
> > > > > >
> > > > > > Pierre-Yves Ritschard
> > > > > >
> > > > > >
> > > > >
> > > > >
> > > >
> > > >
> > > > --
> > > >
> > > >
> > > > *Nate Gordon*Director of Technology | Appcore - the business of cloud
> > > > computing®
> > > >
> > > > Office +1.800.735.7104 | Direct +1.515.612.7787
> > > > nate.gordon@appcore.com | www.appcore.com
> > > >
> > > >
> ----------------------------------------------------------------------
> > > >
> > > > The information in this message is intended for the named recipients
> > > only.
> > > > It may contain information that is privileged, confidential or
> > otherwise
> > > > protected from disclosure. If you are not the intended recipient, you
> > are
> > > > hereby notified that any disclosure, copying, distribution, or the
> > taking
> > > > of any action in reliance on the contents of this message is strictly
> > > > prohibited. If you have received this e-mail in error, do not print
> it
> > or
> > > > disseminate it or its contents. In such event, please notify the
> sender
> > > by
> > > > return e-mail and delete the e-mail file immediately thereafter.
> Thank
> > > you.
> > > >
> > >
> > >
> >
>
>
>
> --
> Olivier Lemasle
> Ingénieur Logiciel
> *Apalia*™
> Mobile: +33-611-69-12-11
>
> *http://www.apalia.net <http://www.apalia.net>
> <ol...@apalia.net>olivier.lemasle@apalia.net
> <ol...@apalia.net>*
>
Re: Review Request 20557: CLOUDSTACK-6472 listUsageRecords generates
NPEs for expunging instances
Posted by Olivier Lemasle <ol...@apalia.net>.
Hi Sam,
I was going to suggest the same patch than you (with
findByIdIncludingRemoved), and I hit the same issue: no method
findByIdIncludingRemoved in com.cloud.utils.db.EntityManager.
But I see that the implementing class com.cloud.dao.EntityManagerImpl still
has findByIdIncludingRemoved(). Do you know why it was removed from the
interface in f5e5b39c9bc8d?
--
Olivier
On Mon, May 5, 2014 at 3:45 PM, Sam Schmit <sa...@appcore.com> wrote:
> Sebastien,
>
> I took a look at the 4.3 code, and I was mistaken - it will not commit
> cleanly. The EntityManager does not have the findIdIncludingRemoved
> function added yet. I'll have to spend more time looking into how to get
> this into 4.3 cleanly.
>
> Sam
>
>
>
> On Fri, May 2, 2014 at 3:57 PM, Sebastien Goasguen <ru...@gmail.com>
> wrote:
>
> >
> > On May 2, 2014, at 2:17 PM, Sam Schmit <sa...@appcore.com> wrote:
> >
> > > All,
> > >
> > > I have submitted a bug fix patch for this issue:
> > >
> > > https://reviews.apache.org/r/21015/diff/
> > >
> > > If approved, this fix should also find its way into the 4.3 and 4.4
> > builds. It is fairly critical for getting usage data out of cloudstack.
> > >
> >
> > Sam, thanks a lot for the patch. Since You and Pyr actually use this in
> > prod, I will rely on your for testing :)
> >
> > I applied your patch to master and 4.4-forward.
> >
> > I need a clean patch for 4.3
> >
> > If you could check 4.4-forward and tell me if it's ok then I will request
> > a cherry-pick shortly (to make the first RC).
> >
> >
> > > Sam Schmit
> > >
> > >
> > > On Thu, May 1, 2014 at 5:35 PM, Nate Gordon <na...@appcore.com>
> > wrote:
> > > Sorry for being late to the discussion, I believe this patch doesn't
> > fully
> > > do what is actually expected. This patch does prevent the NPE, but
> > doesn't
> > > prevent the reason why the NPE was happening in the first place. It
> > appears
> > > that in commit f5e5b39, all the instances of
> > > find{something}ByIdIncludingRemoved(id) where changed to
> > > find{something}ById(id). This was 9 months ago, and since then it was
> > > changed again to _entityMgr.findById({something}.class, id). So the
> > reason
> > > for the NPE is actually that we aren't querying for removed entries,
> but
> > > those entries still exist, and used to be returned by the
> > listUsageRecords
> > > api. This patch mostly just prevents the UUID field from being
> populated
> > in
> > > those instances, which are relied upon by myself and surely others to
> > > correlate records together.
> > >
> > > I'll try to get a new patch put together tonight that addresses that,
> > > unless there is some other commentary as to why
> findByIdIncludingRemoved
> > > was not included in the EntityManager interface, but is still present
> in
> > > EntityManagerImpl.
> > >
> > > Having said that, I was just commenting to someone today that it would
> be
> > > nice if there was some null checking done in this process and am glad
> > that
> > > it exists now. We have instances from time to time where something just
> > > gets flat out removed from the db and that tends to break usage which
> > > requires some unfortunate debugging effort.
> > >
> > > Thanks,
> > >
> > > -Nate
> > >
> > >
> > > On Fri, Apr 25, 2014 at 4:01 PM, Sebastien Goasguen <runseb@gmail.com
> > >wrote:
> > >
> > > >
> > > > -----------------------------------------------------------
> > > > This is an automatically generated e-mail. To reply, visit:
> > > > https://reviews.apache.org/r/20557/#review41524
> > > > -----------------------------------------------------------
> > > >
> > > > Ship it!
> > > >
> > > >
> > > > Applied to 4.3 with commit:
> > > > 08997a9ba37d939dc6e546c632daf93b2b04e825
> > > >
> > > > I re-wrote the patch and committed to master as well with:
> > > > 744e2a54e8b05d8136382664d8e5b9e3649fe88e
> > > >
> > > > Thanks for the patch, please make sure to tell us if there is more
> > issue
> > > > with this.
> > > >
> > > > You can mark the review as submitted.
> > > >
> > > >
> > > > - Sebastien Goasguen
> > > >
> > > >
> > > > On April 23, 2014, 12:47 p.m., Pierre-Yves Ritschard wrote:
> > > > >
> > > > > -----------------------------------------------------------
> > > > > This is an automatically generated e-mail. To reply, visit:
> > > > > https://reviews.apache.org/r/20557/
> > > > > -----------------------------------------------------------
> > > > >
> > > > > (Updated April 23, 2014, 12:47 p.m.)
> > > > >
> > > > >
> > > > > Review request for cloudstack.
> > > > >
> > > > >
> > > > > Bugs: CLOUDSTACK-6472
> > > > > https://issues.apache.org/jira/browse/CLOUDSTACK-6472
> > > > >
> > > > >
> > > > > Repository: cloudstack-git
> > > > >
> > > > >
> > > > > Description
> > > > > -------
> > > > >
> > > > > This is a review request for CLOUDSTACK-6472 "listUsageRecords
> > generates
> > > > NPEs for expunging instances"
> > > > >
> > > > > The patch is against the 4.3 branch
> > > > >
> > > > >
> > > > > Diffs
> > > > > -----
> > > > >
> > > > > server/src/com/cloud/api/ApiResponseHelper.java e543d1c
> > > > >
> > > > > Diff: https://reviews.apache.org/r/20557/diff/
> > > > >
> > > > >
> > > > > Testing
> > > > > -------
> > > > >
> > > > >
> > > > > Thanks,
> > > > >
> > > > > Pierre-Yves Ritschard
> > > > >
> > > > >
> > > >
> > > >
> > >
> > >
> > > --
> > >
> > >
> > > *Nate Gordon*Director of Technology | Appcore - the business of cloud
> > > computing®
> > >
> > > Office +1.800.735.7104 | Direct +1.515.612.7787
> > > nate.gordon@appcore.com | www.appcore.com
> > >
> > > ----------------------------------------------------------------------
> > >
> > > The information in this message is intended for the named recipients
> > only.
> > > It may contain information that is privileged, confidential or
> otherwise
> > > protected from disclosure. If you are not the intended recipient, you
> are
> > > hereby notified that any disclosure, copying, distribution, or the
> taking
> > > of any action in reliance on the contents of this message is strictly
> > > prohibited. If you have received this e-mail in error, do not print it
> or
> > > disseminate it or its contents. In such event, please notify the sender
> > by
> > > return e-mail and delete the e-mail file immediately thereafter. Thank
> > you.
> > >
> >
> >
>
--
Olivier Lemasle
Ingénieur Logiciel
*Apalia*™
Mobile: +33-611-69-12-11
*http://www.apalia.net <http://www.apalia.net>
<ol...@apalia.net>olivier.lemasle@apalia.net
<ol...@apalia.net>*
Re: Review Request 20557: CLOUDSTACK-6472 listUsageRecords generates
NPEs for expunging instances
Posted by Sam Schmit <sa...@appcore.com>.
Sebastien,
I took a look at the 4.3 code, and I was mistaken - it will not commit
cleanly. The EntityManager does not have the findIdIncludingRemoved
function added yet. I'll have to spend more time looking into how to get
this into 4.3 cleanly.
Sam
On Fri, May 2, 2014 at 3:57 PM, Sebastien Goasguen <ru...@gmail.com> wrote:
>
> On May 2, 2014, at 2:17 PM, Sam Schmit <sa...@appcore.com> wrote:
>
> > All,
> >
> > I have submitted a bug fix patch for this issue:
> >
> > https://reviews.apache.org/r/21015/diff/
> >
> > If approved, this fix should also find its way into the 4.3 and 4.4
> builds. It is fairly critical for getting usage data out of cloudstack.
> >
>
> Sam, thanks a lot for the patch. Since You and Pyr actually use this in
> prod, I will rely on your for testing :)
>
> I applied your patch to master and 4.4-forward.
>
> I need a clean patch for 4.3
>
> If you could check 4.4-forward and tell me if it's ok then I will request
> a cherry-pick shortly (to make the first RC).
>
>
> > Sam Schmit
> >
> >
> > On Thu, May 1, 2014 at 5:35 PM, Nate Gordon <na...@appcore.com>
> wrote:
> > Sorry for being late to the discussion, I believe this patch doesn't
> fully
> > do what is actually expected. This patch does prevent the NPE, but
> doesn't
> > prevent the reason why the NPE was happening in the first place. It
> appears
> > that in commit f5e5b39, all the instances of
> > find{something}ByIdIncludingRemoved(id) where changed to
> > find{something}ById(id). This was 9 months ago, and since then it was
> > changed again to _entityMgr.findById({something}.class, id). So the
> reason
> > for the NPE is actually that we aren't querying for removed entries, but
> > those entries still exist, and used to be returned by the
> listUsageRecords
> > api. This patch mostly just prevents the UUID field from being populated
> in
> > those instances, which are relied upon by myself and surely others to
> > correlate records together.
> >
> > I'll try to get a new patch put together tonight that addresses that,
> > unless there is some other commentary as to why findByIdIncludingRemoved
> > was not included in the EntityManager interface, but is still present in
> > EntityManagerImpl.
> >
> > Having said that, I was just commenting to someone today that it would be
> > nice if there was some null checking done in this process and am glad
> that
> > it exists now. We have instances from time to time where something just
> > gets flat out removed from the db and that tends to break usage which
> > requires some unfortunate debugging effort.
> >
> > Thanks,
> >
> > -Nate
> >
> >
> > On Fri, Apr 25, 2014 at 4:01 PM, Sebastien Goasguen <runseb@gmail.com
> >wrote:
> >
> > >
> > > -----------------------------------------------------------
> > > This is an automatically generated e-mail. To reply, visit:
> > > https://reviews.apache.org/r/20557/#review41524
> > > -----------------------------------------------------------
> > >
> > > Ship it!
> > >
> > >
> > > Applied to 4.3 with commit:
> > > 08997a9ba37d939dc6e546c632daf93b2b04e825
> > >
> > > I re-wrote the patch and committed to master as well with:
> > > 744e2a54e8b05d8136382664d8e5b9e3649fe88e
> > >
> > > Thanks for the patch, please make sure to tell us if there is more
> issue
> > > with this.
> > >
> > > You can mark the review as submitted.
> > >
> > >
> > > - Sebastien Goasguen
> > >
> > >
> > > On April 23, 2014, 12:47 p.m., Pierre-Yves Ritschard wrote:
> > > >
> > > > -----------------------------------------------------------
> > > > This is an automatically generated e-mail. To reply, visit:
> > > > https://reviews.apache.org/r/20557/
> > > > -----------------------------------------------------------
> > > >
> > > > (Updated April 23, 2014, 12:47 p.m.)
> > > >
> > > >
> > > > Review request for cloudstack.
> > > >
> > > >
> > > > Bugs: CLOUDSTACK-6472
> > > > https://issues.apache.org/jira/browse/CLOUDSTACK-6472
> > > >
> > > >
> > > > Repository: cloudstack-git
> > > >
> > > >
> > > > Description
> > > > -------
> > > >
> > > > This is a review request for CLOUDSTACK-6472 "listUsageRecords
> generates
> > > NPEs for expunging instances"
> > > >
> > > > The patch is against the 4.3 branch
> > > >
> > > >
> > > > Diffs
> > > > -----
> > > >
> > > > server/src/com/cloud/api/ApiResponseHelper.java e543d1c
> > > >
> > > > Diff: https://reviews.apache.org/r/20557/diff/
> > > >
> > > >
> > > > Testing
> > > > -------
> > > >
> > > >
> > > > Thanks,
> > > >
> > > > Pierre-Yves Ritschard
> > > >
> > > >
> > >
> > >
> >
> >
> > --
> >
> >
> > *Nate Gordon*Director of Technology | Appcore - the business of cloud
> > computing®
> >
> > Office +1.800.735.7104 | Direct +1.515.612.7787
> > nate.gordon@appcore.com | www.appcore.com
> >
> > ----------------------------------------------------------------------
> >
> > The information in this message is intended for the named recipients
> only.
> > It may contain information that is privileged, confidential or otherwise
> > protected from disclosure. If you are not the intended recipient, you are
> > hereby notified that any disclosure, copying, distribution, or the taking
> > of any action in reliance on the contents of this message is strictly
> > prohibited. If you have received this e-mail in error, do not print it or
> > disseminate it or its contents. In such event, please notify the sender
> by
> > return e-mail and delete the e-mail file immediately thereafter. Thank
> you.
> >
>
>
Re: Review Request 20557: CLOUDSTACK-6472 listUsageRecords generates NPEs for expunging instances
Posted by Sebastien Goasguen <ru...@gmail.com>.
On May 2, 2014, at 2:17 PM, Sam Schmit <sa...@appcore.com> wrote:
> All,
>
> I have submitted a bug fix patch for this issue:
>
> https://reviews.apache.org/r/21015/diff/
>
> If approved, this fix should also find its way into the 4.3 and 4.4 builds. It is fairly critical for getting usage data out of cloudstack.
>
Sam, thanks a lot for the patch. Since You and Pyr actually use this in prod, I will rely on your for testing :)
I applied your patch to master and 4.4-forward.
I need a clean patch for 4.3
If you could check 4.4-forward and tell me if it's ok then I will request a cherry-pick shortly (to make the first RC).
> Sam Schmit
>
>
> On Thu, May 1, 2014 at 5:35 PM, Nate Gordon <na...@appcore.com> wrote:
> Sorry for being late to the discussion, I believe this patch doesn't fully
> do what is actually expected. This patch does prevent the NPE, but doesn't
> prevent the reason why the NPE was happening in the first place. It appears
> that in commit f5e5b39, all the instances of
> find{something}ByIdIncludingRemoved(id) where changed to
> find{something}ById(id). This was 9 months ago, and since then it was
> changed again to _entityMgr.findById({something}.class, id). So the reason
> for the NPE is actually that we aren't querying for removed entries, but
> those entries still exist, and used to be returned by the listUsageRecords
> api. This patch mostly just prevents the UUID field from being populated in
> those instances, which are relied upon by myself and surely others to
> correlate records together.
>
> I'll try to get a new patch put together tonight that addresses that,
> unless there is some other commentary as to why findByIdIncludingRemoved
> was not included in the EntityManager interface, but is still present in
> EntityManagerImpl.
>
> Having said that, I was just commenting to someone today that it would be
> nice if there was some null checking done in this process and am glad that
> it exists now. We have instances from time to time where something just
> gets flat out removed from the db and that tends to break usage which
> requires some unfortunate debugging effort.
>
> Thanks,
>
> -Nate
>
>
> On Fri, Apr 25, 2014 at 4:01 PM, Sebastien Goasguen <ru...@gmail.com>wrote:
>
> >
> > -----------------------------------------------------------
> > This is an automatically generated e-mail. To reply, visit:
> > https://reviews.apache.org/r/20557/#review41524
> > -----------------------------------------------------------
> >
> > Ship it!
> >
> >
> > Applied to 4.3 with commit:
> > 08997a9ba37d939dc6e546c632daf93b2b04e825
> >
> > I re-wrote the patch and committed to master as well with:
> > 744e2a54e8b05d8136382664d8e5b9e3649fe88e
> >
> > Thanks for the patch, please make sure to tell us if there is more issue
> > with this.
> >
> > You can mark the review as submitted.
> >
> >
> > - Sebastien Goasguen
> >
> >
> > On April 23, 2014, 12:47 p.m., Pierre-Yves Ritschard wrote:
> > >
> > > -----------------------------------------------------------
> > > This is an automatically generated e-mail. To reply, visit:
> > > https://reviews.apache.org/r/20557/
> > > -----------------------------------------------------------
> > >
> > > (Updated April 23, 2014, 12:47 p.m.)
> > >
> > >
> > > Review request for cloudstack.
> > >
> > >
> > > Bugs: CLOUDSTACK-6472
> > > https://issues.apache.org/jira/browse/CLOUDSTACK-6472
> > >
> > >
> > > Repository: cloudstack-git
> > >
> > >
> > > Description
> > > -------
> > >
> > > This is a review request for CLOUDSTACK-6472 "listUsageRecords generates
> > NPEs for expunging instances"
> > >
> > > The patch is against the 4.3 branch
> > >
> > >
> > > Diffs
> > > -----
> > >
> > > server/src/com/cloud/api/ApiResponseHelper.java e543d1c
> > >
> > > Diff: https://reviews.apache.org/r/20557/diff/
> > >
> > >
> > > Testing
> > > -------
> > >
> > >
> > > Thanks,
> > >
> > > Pierre-Yves Ritschard
> > >
> > >
> >
> >
>
>
> --
>
>
> *Nate Gordon*Director of Technology | Appcore - the business of cloud
> computing®
>
> Office +1.800.735.7104 | Direct +1.515.612.7787
> nate.gordon@appcore.com | www.appcore.com
>
> ----------------------------------------------------------------------
>
> The information in this message is intended for the named recipients only.
> It may contain information that is privileged, confidential or otherwise
> protected from disclosure. If you are not the intended recipient, you are
> hereby notified that any disclosure, copying, distribution, or the taking
> of any action in reliance on the contents of this message is strictly
> prohibited. If you have received this e-mail in error, do not print it or
> disseminate it or its contents. In such event, please notify the sender by
> return e-mail and delete the e-mail file immediately thereafter. Thank you.
>
Re: Review Request 20557: CLOUDSTACK-6472 listUsageRecords generates
NPEs for expunging instances
Posted by Sam Schmit <sa...@appcore.com>.
All,
I have submitted a bug fix patch for this issue:
https://reviews.apache.org/r/21015/diff/
If approved, this fix should also find its way into the 4.3 and 4.4 builds.
It is fairly critical for getting usage data out of cloudstack.
Sam Schmit
On Thu, May 1, 2014 at 5:35 PM, Nate Gordon <na...@appcore.com> wrote:
> Sorry for being late to the discussion, I believe this patch doesn't fully
> do what is actually expected. This patch does prevent the NPE, but doesn't
> prevent the reason why the NPE was happening in the first place. It appears
> that in commit f5e5b39, all the instances of
> find{something}ByIdIncludingRemoved(id) where changed to
> find{something}ById(id). This was 9 months ago, and since then it was
> changed again to _entityMgr.findById({something}.class, id). So the reason
> for the NPE is actually that we aren't querying for removed entries, but
> those entries still exist, and used to be returned by the listUsageRecords
> api. This patch mostly just prevents the UUID field from being populated in
> those instances, which are relied upon by myself and surely others to
> correlate records together.
>
> I'll try to get a new patch put together tonight that addresses that,
> unless there is some other commentary as to why findByIdIncludingRemoved
> was not included in the EntityManager interface, but is still present in
> EntityManagerImpl.
>
> Having said that, I was just commenting to someone today that it would be
> nice if there was some null checking done in this process and am glad that
> it exists now. We have instances from time to time where something just
> gets flat out removed from the db and that tends to break usage which
> requires some unfortunate debugging effort.
>
> Thanks,
>
> -Nate
>
>
> On Fri, Apr 25, 2014 at 4:01 PM, Sebastien Goasguen <runseb@gmail.com
> >wrote:
>
> >
> > -----------------------------------------------------------
> > This is an automatically generated e-mail. To reply, visit:
> > https://reviews.apache.org/r/20557/#review41524
> > -----------------------------------------------------------
> >
> > Ship it!
> >
> >
> > Applied to 4.3 with commit:
> > 08997a9ba37d939dc6e546c632daf93b2b04e825
> >
> > I re-wrote the patch and committed to master as well with:
> > 744e2a54e8b05d8136382664d8e5b9e3649fe88e
> >
> > Thanks for the patch, please make sure to tell us if there is more issue
> > with this.
> >
> > You can mark the review as submitted.
> >
> >
> > - Sebastien Goasguen
> >
> >
> > On April 23, 2014, 12:47 p.m., Pierre-Yves Ritschard wrote:
> > >
> > > -----------------------------------------------------------
> > > This is an automatically generated e-mail. To reply, visit:
> > > https://reviews.apache.org/r/20557/
> > > -----------------------------------------------------------
> > >
> > > (Updated April 23, 2014, 12:47 p.m.)
> > >
> > >
> > > Review request for cloudstack.
> > >
> > >
> > > Bugs: CLOUDSTACK-6472
> > > https://issues.apache.org/jira/browse/CLOUDSTACK-6472
> > >
> > >
> > > Repository: cloudstack-git
> > >
> > >
> > > Description
> > > -------
> > >
> > > This is a review request for CLOUDSTACK-6472 "listUsageRecords
> generates
> > NPEs for expunging instances"
> > >
> > > The patch is against the 4.3 branch
> > >
> > >
> > > Diffs
> > > -----
> > >
> > > server/src/com/cloud/api/ApiResponseHelper.java e543d1c
> > >
> > > Diff: https://reviews.apache.org/r/20557/diff/
> > >
> > >
> > > Testing
> > > -------
> > >
> > >
> > > Thanks,
> > >
> > > Pierre-Yves Ritschard
> > >
> > >
> >
> >
>
>
> --
>
>
> *Nate Gordon*Director of Technology | Appcore - the business of cloud
> computing®
>
> Office +1.800.735.7104 | Direct +1.515.612.7787
> nate.gordon@appcore.com | www.appcore.com
>
> ----------------------------------------------------------------------
>
> The information in this message is intended for the named recipients only.
> It may contain information that is privileged, confidential or otherwise
> protected from disclosure. If you are not the intended recipient, you are
> hereby notified that any disclosure, copying, distribution, or the taking
> of any action in reliance on the contents of this message is strictly
> prohibited. If you have received this e-mail in error, do not print it or
> disseminate it or its contents. In such event, please notify the sender by
> return e-mail and delete the e-mail file immediately thereafter. Thank you.
>