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

Return 0 by default on worker's try_lock_asset() #1478

Closed
wants to merge 1 commit into from

Conversation

mudler
Copy link
Member

@mudler mudler commented Oct 17, 2017

In case of database locks, we attempt to rollback, but we are returning
the $dbh->rollback() result, which leads to unexpected results. In this
way we return 0 by default, and result otherwise. This is still far from
a correct solution since it can still go into deep loop.

Besides, toggle_asset_lock() can be potentially subject to the same
issue.

See related Progress issue: https://progress.opensuse.org/issues/26088

In case of database locks, we attempt to rollback, but we are returning
the $dbh->rollback() result, which leads to unexpected results. In this
way we return 0 by default, and result otherwise. This is still far from
a correct solution since it can still go into deep loop.

Besides, toggle_asset_lock() can be potentially subject to the same
issue.

See related Progress issue: https://progress.opensuse.org/issues/26088
@coolo
Copy link
Contributor

coolo commented Oct 17, 2017

looks reasonable to me

@codecov
Copy link

codecov bot commented Oct 17, 2017

Codecov Report

Merging #1478 into master will decrease coverage by <.01%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1478      +/-   ##
==========================================
- Coverage   88.21%   88.21%   -0.01%     
==========================================
  Files         105      105              
  Lines        7959     7958       -1     
==========================================
- Hits         7021     7020       -1     
  Misses        938      938
Impacted Files Coverage Δ
lib/OpenQA/Worker/Cache.pm 86.04% <50%> (-0.07%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 41b57ae...286dec5. Read the comment docs.

@mudler mudler changed the title [WIP] Return 0 by default on worker's try_lock_asset() Return 0 by default on worker's try_lock_asset() Oct 18, 2017
@coolo
Copy link
Contributor

coolo commented Oct 18, 2017

merged in 1467

@coolo coolo closed this Oct 18, 2017
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.

None yet

2 participants