cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Olivier Lemasle <olivier.lema...@apalia.net>
Subject Re: Review Request 20557: CLOUDSTACK-6472 listUsageRecords generates NPEs for expunging instances
Date Mon, 05 May 2014 15:04:37 GMT
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 <sam.schmit@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 <runseb@gmail.com>
> wrote:
>
> >
> > 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.
> > >
> >
> >
>



-- 
Olivier Lemasle
Ingénieur Logiciel
*Apalia*™
Mobile: +33-611-69-12-11

*http://www.apalia.net <http://www.apalia.net>
<olivier.lemasle@apalia.net>olivier.lemasle@apalia.net
<olivier.lemasle@apalia.net>*

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message