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.lema...@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 <sam.sch...@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 <run...@gmail.com> > > wrote: > > > > > > > > On May 2, 2014, at 2:17 PM, Sam Schmit <sam.sch...@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.gor...@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 < > run...@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.gor...@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.lema...@apalia.net>olivier.lema...@apalia.net > <olivier.lema...@apalia.net>* >
