Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[16.0][FIX] account_financial_report: optimize computation of analytic accounts #1105

Merged
merged 1 commit into from
Feb 1, 2024
Merged

[16.0][FIX] account_financial_report: optimize computation of analytic accounts #1105

merged 1 commit into from
Feb 1, 2024

Conversation

yajo
Copy link
Member

@yajo yajo commented Feb 1, 2024

Installing the module in a big DB was very slow.

I applied several optimizations to the method for working better with big datasets:

  • Prefetch all analytic account distribution.
  • Batch writes as much as possible.

@moduon MT-4982

@rafaelbn rafaelbn added this to the 16.0 milestone Feb 1, 2024
@rafaelbn
Copy link
Member

rafaelbn commented Feb 1, 2024

Please @norlinhenrik @gurneyalex @rousseldenis could take a look here? We cannot install in a BIG 16 DB. Thanks! 😄 ❤️

rafaelbn
rafaelbn approved these changes Feb 1, 2024
View reviewed changes
Copy link
Member

@rafaelbn rafaelbn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested in production 👍🏼

👍🏼 Now I can install
👎🏼 Before never install

edlopen
edlopen approved these changes Feb 1, 2024
View reviewed changes
Copy link
Member

@edlopen edlopen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@norlinhenrik
Copy link

Nice performance improvement!

In my code review, I first thought that account referred to regular account. I would prefer using the terms batch_by_analytic_account and analytic_account_id to be clear.

@pedrobaeza
Copy link
Member

Check also #1081

@pedrobaeza pedrobaeza changed the title [FIX] account_financial_report: optimize computation of analytic accounts [16.0][FIX] account_financial_report: optimize computation of analytic accounts Feb 1, 2024
@yajo
[FIX] account_financial_report: optimize computation of analytic acco…
f4e7e2a 
…unts

Installing the module in a big DB was very slow.

I applied several optimizations to the method for working better with big datasets:
- Prefetch all analytic account distribution.
- Batch writes as much as possible.

@moduon MT-4982
@yajo yajo force-pushed the 16.0-account_financial_report-improve_performance branch from 807e171 to f4e7e2a Compare February 1, 2024 12:52
@yajo
Copy link
Member Author

yajo commented Feb 1, 2024

@norlinhenrik applied.

@pedrobaeza I saw that, but it's a draft and involves behavioral changes. This one is just a performance optimization that can improve the current implementation, independently of what's decided there finally.

pedrobaeza
pedrobaeza approved these changes Feb 1, 2024
View reviewed changes
Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, let's go with it!

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 16.0-ocabot-merge-pr-1105-by-pedrobaeza-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 5c08aa4 into OCA:16.0 Feb 1, 2024
5 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at b2d4856. Thanks a lot for contributing to OCA. ❤️

@yajo yajo deleted the 16.0-account_financial_report-improve_performance branch February 2, 2024 08:43
@victoralmau victoralmau mentioned this pull request Feb 22, 2024
Merged
@rousseldenis rousseldenis mentioned this pull request Apr 2, 2024
Draft
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pedrobaeza pedrobaeza pedrobaeza approved these changes

@rafaelbn rafaelbn rafaelbn approved these changes

@edlopen edlopen edlopen approved these changes

@EmilioPascual EmilioPascual EmilioPascual approved these changes

Assignees
No one assigned
Labels
approved merged 🎉 needs review
Projects
None yet
Milestone
16.0
Development

Successfully merging this pull request may close these issues.
7 participants
@yajo @rafaelbn @norlinhenrik @pedrobaeza @OCA-git-bot @edlopen @EmilioPascual