cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Sebastien Goasguen <run...@gmail.com>
Subject Re: Review Request 20557: CLOUDSTACK-6472 listUsageRecords generates NPEs for expunging instances
Date Fri, 02 May 2014 20:57:59 GMT

On May 2, 2014, at 2:17 PM, Sam Schmit <sam.schmit@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.
> 


Mime
View raw message