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 <sam.sch...@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.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>* >> > >
