-
-
Notifications
You must be signed in to change notification settings - Fork 534
[10.0][IMP] queue_job: Don't prefetch fields when vacuum + bumpversion #115
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
Conversation
|
@guewen I bumped version as it was not done for long. But in separate commit if you disagree. |
queue_job/models/queue_job.py
Outdated
| """ | ||
| deadline = datetime.now() - timedelta(days=self._removal_interval) | ||
| jobs = self.search( | ||
| jobs = self.with_context(prefetch_fields=False).search( |
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 was sure that the prefetch only happen the first time you access a field (other than id)?
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 will try to objective that with a test script (not in the module)
2dc130c to
e44038d
Compare
|
@guewen I think you're right. |
queue_job/models/queue_job.py
Outdated
| job_obj = self.env['queue.job'] | ||
| deadline = datetime.now() - timedelta(days=self._removal_interval) | ||
| jobs = self.search( | ||
| jobs_ids = self.with_context(prefetch_fields=True).search( |
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.
@rousseldenis IMO prefetch_field is useless
| cr.commit() | ||
| except OperationalError: | ||
| if use_new_cursor: | ||
| cr.rollback() |
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.
@rousseldenis IMO all the logic should be enclosed into a try except finally block
if use_new_cursor:
cr = registry(self._cr.dbname).cursor()
try:
self = self.with_env(self.env(cr=cr))
jobs = job_obj.browse(jobs_ids[:jobs_set])
jobs_ids = jobs_ids[jobs_set:]
jobs.unlink()
if use_new_cursor:
cr.commit()
except OperationalError:
if use_new_cursor:
cr.rollback()
raise
finally:
if use_new_cursor:
cr.close()e44038d to
b5d8160
Compare
b5d8160 to
c4e8caf
Compare
lmignon
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.
@rousseldenis Can you modify the changlog PLZ https://github.com/OCA/queue/blob/10.0/queue_job/README.rst#changelog
|
@rousseldenis are you sure the benefit is worth the additional code complexity? |
|
@sbidoul You have to handle also mail messages. So, in fact there should be multiple sql deletes. |
|
There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. |
@guewen @lmignon
Did this as sometimes, the amount of jobs is huge and there is no need fetching fields.