jamessimone / apex-rollup

Fast, configurable, elastically scaling custom rollup solution. Apex Invocable action, one-liner Apex trigger/CMDT-driven logic, and scheduled Apex-ready.
MIT License
210 stars 30 forks source link

Full Recalculation Grandparent Rollup not calculating SUM correctly of all child records #523

Closed baobao917 closed 9 months ago

baobao917 commented 10 months ago

Hi @jamessimone - Opening a new issue, as discussed, separate from #500 .

After implementing v.1.6.0, I tested the full recalculation from the Recalculate Rollup tab on a grandparent rollup. I then ran a report summing the grandchild record amounts and compared them against the grandparent actual rollup calculated amount. Some did calculate correctly, but a fairly large number did not. See below the details:

Video - https://www.loom.com/share/b12ea6de638241c6a4a1a61d441ff2a9 From video - Exported report from production (excel) - Asset Plan Spend Grandparent Rollup QA calculation.xlsx

I believe you have access to the sandbox org still. I created the same report in sandbox (as shown in video). Link is below https://serviceone--uat.sandbox.lightning.force.com/lightning/r/Report/00O7e000001A9HhEAK/view?queryScope=userFolders

jamessimone commented 10 months ago

@baobao917 finally had the chance to break ground on this issue today. Many thanks, again, for your videos and writeups. As mentioned on #500, grandparent full recalcs work slightly differently from standard rollups, which is why I thought it best to break this out into a separate issue. I'm still hopeful to include this fix in 1.6.1 - I will keep you updated as to my progress.

baobao917 commented 10 months ago

Thanks @jamessimone . I'm going to have to re-open #500 as well. I went thru an exercise (similar as to the one here for grandparent rollup full recalculations) to validate the parent rollup full recalculations for all of the parent records.

jamessimone commented 10 months ago

@baobao917 I started looking into this just now, starting with this Asset record (which is the first one shown in the report you linked). By default, Apex Rollup lets updates silently fail when there's an issue updating a parent record. This is colloquially described as the "all or none" database setting, where a true value shows exceptions on save and a false value (what Apex Rollup uses) leads to this silent failure. If I temporarily update the default value for "all or none" to true, here's what I see:

FATAL_ERROR System.DmlException: Update failed. First exception on row 0 with id 02i4w00000Z47H6AAJ; 
first error: REQUIRED_FIELD_MISSING, Required fields are missing: [Equipment Type]: [Equipment Type]

I get the same error with the second row in your report as well. Any chance the Assets that weren't properly updated are all missing this required value?

baobao917 commented 10 months ago

Hi James - I can def check that and will let you know. Where is this setting "all or none"? Will it throw an error if I set this setting (and if so, how? Via email?)

Also if I understand correctly, then the asset record shouldn't have had any roll-up calculation populated into the roll-up field correct? (Since that exception would have prevented the parent record from saving?)

Bc I was definitely seeing some records where the roll-up field was populated but with an incorrect value.

BDO 10/25 20:04 PM- Follow-up: It looks like there are required fields on the parent object (Asset) that need to be populated (Equipment Type, Account and/or Contact). Let me try updating all the asset records in sandbox and production with those required values, then try rerunning the full recalculation. Then I'll see what happens and get back to you.

P.S. I tweaked that SF report a bit so it displays those required fields as well as the 18 digit Id.

On Wed, Oct 25, 2023, 6:32 PM James Simone @.***> wrote:

@baobao917 https://github.com/baobao917 I started looking into this just now, starting with this Asset record https://serviceone--uat.sandbox.lightning.force.com/lightning/r/Asset/02i4w00000Z47H6AAJ/view. By default, Apex Rollup lets updates silently fail when there's an issue updating a parent record. This is colloquially described as the "all or none" database setting, where a true value shows exceptions on save and a false value (what Apex Rollup uses) leads to this silent failure. If I temporarily update the default value for "all or none" to true, here's what I see:

FATAL_ERROR System.DmlException: Update failed. First exception on row 0 with id 02i4w00000Z47H6AAJ; first error: REQUIRED_FIELD_MISSING, Required fields are missing: [Equipment Type]: [Equipment Type]

I get the same error with the second row in your report as well. Any chance the Assets that weren't properly updated are all missing this required value?

— Reply to this email directly, view it on GitHub https://github.com/jamessimone/apex-rollup/issues/523#issuecomment-1780147669, or unsubscribe https://github.com/notifications/unsubscribe-auth/AY7A4OZ63C6U4LIQ4C4YPL3YBGHQDAVCNFSM6AAAAAA6IDWQBKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOBQGE2DONRWHE . You are receiving this because you were mentioned.Message ID: @.***>

jamessimone commented 10 months ago

@baobao917 at the moment "all or none" isn't an exposed setting within Apex Rollup. I could change that in the future.

Yes, your understanding is correct - Apex Rollup would only have been able to update records where the parent record can be saved successfully. It could just be that the first two records I checked happened to be missing that field and that there were legitimate errors lower down on the report - happy to give that one a look over once you've updated the assets!

baobao917 commented 10 months ago

@jamessimone Yes it would be nice if we could know if something was preventing the rollup from calculating correctly.

I probably won't be able to update the Asset record until this Friday or next week.

baobao917 commented 10 months ago

@jamessimone In case you don't want to read the summary below, I recorded a video

https://www.loom.com/share/507d6e7495f14683be1449868558022b?sid=e2d21cfe-fe5d-4bf4-b263-67accdb37480

So based on your information, I did the following:

In the sandbox, the calculations were correct and agreed.

However, I went thru the same exercise in production and I still have many (3,020) exceptions. Below is the files and links from production. Production report - https://serviceone.lightning.force.com/lightning/r/Report/00O4w000009AUwqEAG/view?queryScope=userFolders Production excel file - Asset Plan Spend Grandparent Rollup QA calculation-PROD v2.xlsx

So since the exceptions exist in production, I'm not sure how you wish for me to proceed here. If you need access, just let me know and I'll have to get authorization.

jamessimone commented 10 months ago

Interesting. I haven't had the chance to take a look yet. I am planning to come back to this one - going to wrap up the next release with the full fix for #518 tomorrow. The only thing I can think of, at the moment, is trying the full recalc again with a smaller batch size to see if there were any timeout issues causing some of the grandparent records to not be updated. It could also be that there are other validation errors preventing the Assets from saving, as was the case in the sandbox. I won't be able to get back to investigating this until the week of November 6th.

baobao917 commented 10 months ago

@jamessimone As an FYI, I did check and know it's not any validation errors b/c I ran a simple Update ID operation on all of the exception records and it saved without any issue.

Are you referring to the batch chunk size in the Rollup Control? Suggestions as to what size I should try?

image

jamessimone commented 10 months ago

Yes, I'd try something lower, like 200, and see if that makes a difference. It'll be a good data point, either way.

baobao917 commented 10 months ago

@jamessimone I tried a batch chunk size = 200. Actually have more exceptions now than before (3,538 vs 3,020)...but that could be due to more data as this is a production org.

Thanks for the input....I'll look forward to when you are able to get to this. Thanks!

jamessimone commented 10 months ago

@baobao917 that's curious, the batch size shouldn't change anything other than throttling the overall number of parent records getting updated - which, in an automation-heavy org, would give you more breathing room. It doesn't affect the logic for how rollups are calculated. I suppose that if I had access to the org, I could at least get into the logs and examine a few of the parent records not properly updating to get a better understanding of what the issue is.

baobao917 commented 10 months ago

@jamessimone I will work on getting you access to that org. Will be in touch

baobao917 commented 10 months ago

@jamessimone I will be providing you your own user login for this org. You should be getting an email shortly. Just pls remember that this is a production org. Thanks!

jamessimone commented 10 months ago

@baobao917 I can confirm I got in. I will be reviewing the report you linked previously and running a full recalc to check logs tomorrow

jamessimone commented 10 months ago

@baobao917 - I ran a few recalcs today. As far as I can tell, this is once again an issue with some sort of unrelated DML exception on the parent records. I never saw evidence of a parent record not having the right value get calculated; some of the parents clearly had been updated at some point in the past with a previously correct value, but that now-stale value hasn't been able to be updated by any of the subsequent full recalcs due to whatever silent DML failure is occurring. I am going to be adding some logging in the next version that will, when logging is enabled, create ERROR logging level entries that you can examine after the fact to get insight into the nature of what is preventing each parent record from being updated.

baobao917 commented 10 months ago

I thought it was strange that if the grandparent record had some DML exception that silently failed and prevented it from getting updated, then I would have expected it to be a $0 value. However the fact it had any value at all meant it was able to be updated during the recalc. For context, prior to running the full recalc all of the grandparent target roll-up field had a null value bc this is a brand new field which would t have had any value populated prior to the ApexRollup full recalc job.

Does that make sense?

Thanks, Bao

On Fri, Nov 10, 2023, 2:41 PM James Simone @.***> wrote:

@baobao917 https://github.com/baobao917 - I ran a few recalcs today. As far as I can tell, this is once again an issue with some sort of unrelated DML exception on the parent records. I never saw evidence of a parent record not having the right value get calculated; some of the parents clearly had been updated at some point in the past with a previously correct value, but that now-stale value hasn't been able to be updated by any of the subsequent full recalcs due to whatever silent DML failure is occurring. I am going to be adding some logging in the next version that will, when logging is enabled, create ERROR logging level entries that you can examine after the fact to get insight into the nature of what is preventing each parent record from being updated.

— Reply to this email directly, view it on GitHub https://github.com/jamessimone/apex-rollup/issues/523#issuecomment-1806335128, or unsubscribe https://github.com/notifications/unsubscribe-auth/AY7A4O6TGGJ3WBGLCKTVYPDYDZ7O3AVCNFSM6AAAAAA6IDWQBKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBWGMZTKMJSHA . You are receiving this because you were mentioned.Message ID: @.***>

jamessimone commented 10 months ago

That does make sense, but could there have been other validation rule or new field changes going on during the initial full recalc operation? That's the only explanation I can offer; as far as what I saw, the logs contained the right values (the ones that you had in your spreadsheet), but once v1.6.2 is released we'll also be able to see what - if any - is being reported by the database for record update failures

baobao917 commented 9 months ago

@jamessimone I've upgraded production to v1.6.2. Pls let me know if you need anything from me. Thanks!

jamessimone commented 9 months ago

@baobao917 I can run a full recalc there tomorrow if you'd like. I will just be looking to go through the logs generated by those runs to see which parent-level records are having DML issues with updating.

baobao917 commented 9 months ago

Yes please feel free to run a full recalc on that particular roll up. No one is using that data yet bc I've told the client issues are still being worked out.

Just not on the other roll ups pls. Got those squared away. Thanks!

On Sun, Nov 12, 2023, 9:25 PM James Simone @.***> wrote:

@baobao917 https://github.com/baobao917 I can run a full recalc there tomorrow if you'd like. I will just be looking to go through the logs generated by those runs to see which parent-level records are having DML issues with updating.

— Reply to this email directly, view it on GitHub https://github.com/jamessimone/apex-rollup/issues/523#issuecomment-1807381247, or unsubscribe https://github.com/notifications/unsubscribe-auth/AY7A4O5JA5HB66BN3FXAZL3YEGAIRAVCNFSM6AAAAAA6IDWQBKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBXGM4DCMRUG4 . You are receiving this because you were mentioned.Message ID: @.***>

jamessimone commented 9 months ago

@baobao917 I was able to find the source of the issue while running the full recalc this morning. I'll include a fix for this in the next version

baobao917 commented 9 months ago

Awesome, thanks!

Bao Do, CPA

Principal

Accretive Advisory LLC Annapolis, MD 21403

410-541-6082 | @.***

/in/bdo0917 https://www.linkedin.com/in/bdo0917/

https://trailblazer.me/id/baodo

On Mon, Nov 13, 2023 at 10:16 AM James Simone @.***> wrote:

@baobao917 https://github.com/baobao917 I was able to find the source of the issue while running the full recalc this morning. I'll include a fix for this in the next version

— Reply to this email directly, view it on GitHub https://github.com/jamessimone/apex-rollup/issues/523#issuecomment-1808357948, or unsubscribe https://github.com/notifications/unsubscribe-auth/AY7A4OYTFRIYEMUJAPXCWNTYEI2UDAVCNFSM6AAAAAA6IDWQBKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBYGM2TOOJUHA . You are receiving this because you were mentioned.Message ID: @.***>

baobao917 commented 9 months ago

@jamessimone I tested this full recalc issue on grandparent rollups against v1.6.3. It significantly improved the problem but there were still a small number of records where it still didn't calculate. It didn't appear to have anything to do with any type of validations that prevented the grandparent record from saving. I recorded a video and the two excel file calculations I referenced in the video.

Video - https://www.loom.com/share/1dbe7a8bb62447adb69e09e488e281c5

Excel File (after 1st full recalc) - Asset Plan Spend Grandparent Rollup QA calculation-PROD v3.xlsx

Excel File (after 2nd full recalc) - Asset Plan Spend Grandparent Rollup QA calculation-PROD v4.xlsx

I can certainly handle these manually on my own but wanted to bring to your attention in case it was an indicator of something systemic that is causing an error in recalc.