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.

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

[10.0] retry_postpone using new cursor #41

Merged
merged 11 commits into from
May 25, 2018
Merged

[10.0] retry_postpone using new cursor #41

merged 11 commits into from
May 25, 2018

Conversation

2zx
Copy link

@2zx 2zx commented Dec 2, 2017

I've experienced this error

current transaction is aborted, commands ignored until end of transaction block

trying to requeue a job in case of a failure due to SQL errors (typically concurrency errors)
I think the problem can be solved with the use of a new cr as it is done in the case of a failed job

job.store()
env.cr.commit()

job.env.cr.rollback()
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't you rollback only in case of errors? /cc @guewen

Copy link
Member

Choose a reason for hiding this comment

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

I think it's there to prevent a conflict between the job.env's transaction and the new one. The job record might have been updated by the first one and will be updated by the second one? It's not called in the FailedJobError case though so why is it necessary here?

Copy link
Member

@guewen guewen left a comment

Choose a reason for hiding this comment

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

Do you use this in production? Since when and do you see any side-effect?

job.store()
env.cr.commit()

job.env.cr.rollback()
Copy link
Member

Choose a reason for hiding this comment

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

I think it's there to prevent a conflict between the job.env's transaction and the new one. The job record might have been updated by the first one and will be updated by the second one? It's not called in the FailedJobError case though so why is it necessary here?

@@ -188,7 +188,8 @@ def urlopen():
try:
# we are not interested in the result, so we set a short timeout
# but not too short so we trap and log hard configuration errors
response = requests.get(url, timeout=1)
# CGT: timeout must be hight enought to wait for server startup
response = requests.get(url, timeout=15)
Copy link
Member

Choose a reason for hiding this comment

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

Why is it required for you? What's your setup?

Copy link
Member

@yvaucher yvaucher left a comment

Choose a reason for hiding this comment

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

Tried it and works fine

I didn't experience anymore

current transaction is aborted, commands ignored until end of transaction block

@guewen
Copy link
Member

guewen commented Mar 2, 2018

Great thanks @yvaucher for the test and @2zx for the patch!
@2zx would you mind to remove the change on response = requests.get(url, timeout=15) so we keep only the correction related to the current transaction is aborted, commands ignored until end of transaction block error?

@yvaucher
Copy link
Member

yvaucher commented May 24, 2018

@2zx Could you make the autovacuum improvment on an other PR?

By the way there is still an open question from @guewen about the changes on the timeout.

Are you willing to finalize this Pull request?

@2zx
Copy link
Author

2zx commented May 24, 2018

@yvaucher all unnecessary commits to this pull request have been reverted, sorry for the delay

@coveralls
Copy link

coveralls commented May 24, 2018

Coverage Status

Coverage increased (+0.1%) to 78.182% when pulling b504045 on cogitoweb:10.0 into 8a98721 on OCA:10.0.

Copy link
Member

@guewen guewen left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your contribution!

@guewen
Copy link
Member

guewen commented May 24, 2018

Note: to be squashed on merge

@guewen guewen merged commit 13bdb25 into OCA:10.0 May 25, 2018
@2zx 2zx deleted the 10.0 branch May 25, 2018 12:55
guewen added a commit to guewen/queue that referenced this pull request Apr 2, 2019
If the job's transaction fail with an OperationalError such as a
contraint failure, the transaction is broken and we will not be able
to postpone the job. Open a new transaction to change the job's state.

Port of:

- OCA#41 from @2zx
- OCA#130 from @liweijie0812
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants