-
Notifications
You must be signed in to change notification settings - Fork 0
[RFR] Failsafe and verbose #1
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.
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
[RFR] Failsafe and verbose #1
Conversation
| try: | ||
| record._compute_func_string() | ||
| except KeyError: # unknown model or non-compliant arguments | ||
| error += 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please log the error so that I see what's going wrong here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's this one OCA#341 (comment) but I've pushed a logging commit. Restarting the migration now with this code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm attaching the relevant logging to the ticket.
|
Applying the operation on all 200000 jobs takes about 6 minutes, so that is alright. I made a big hooha about operations on larger amounts of jobs, but removing the jobrunner trigger from the table solved that later on. |
9ec557e to
8ef593b
Compare
|
Replaced deprecated logger.warn by logger.warning |
|
Can you please complete your review? Or are you not considering merging this at all? |
hbrunn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought this was meant just for debugging as you PRed on the private repo, but this will probably be useful for other people too.
I still don't really get it why we care for done jobs at all, but then I don't have much experience with things you might want to do with old jobs.
|
We care for done jobs because of code like https://github.com/VanMoof/odoo-addons/blob/12.0/vanmoof_magento/models/magento_sale_order.py#L27-L34. I will be proposing to override this code again with a custom change that defers recomputing func_string here. So for general usefulness I would change this PR to remove the logging of the tracebacks on every KeyError, and if you want I can restore the filter on non-done jobs. Please let me know if that has your preference. |
|
ah, I see. No worries about non-done jobs, I've no claims here |
8ef593b to
89d5fc9
Compare
|
Removed exc_info=True from the debug log, so whenever you are ready you can cherry-pick or rebase this to make it show up in your public OCA PR. |
|
thanks, as it's now a part of OCA#341 I'll close this |
Tentative patch that I'm now running the migration with.