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.
