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. >
