Skip to content

[enhancement] Check task exists in db and requirements upgrade #1361

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

Merged
merged 11 commits into from
Mar 12, 2020

Conversation

pcrespov
Copy link
Member

@pcrespov pcrespov commented Mar 11, 2020

What do these changes do?

Due to a "race condition fix" done for the comp-tasks table, we introduced a additional constraint in the db table. This constraint was also "abused" to check whether a task was already in place or not. At the time this seemed to me an efficient way of "check and if it does not exists, create" in one call. The drawback is that it would log lots of irrelevant error messages in the postgres server. This type of noise was hiding real issues.

Now instead, we split in two calls: pre-check task existence before creating in comp-tasks table. It adds one call but the errors are only logged when we have race condition. No

some minor extras

  • Upgrades test requirements: overcomes vulnerabilities (codecov)
  • Minor tuning of sc-pg discover and added Makefile recipe for dev
  • Re-tunes again log levels

How to test

Run a tutorial. Tasks should be correctly updated in comp-task table

Checklist

  • Did you change any service's API? Then make sure to bundle document and upgrade version (make openapi-specs, git commit ... and then make version-*)
  • Unit tests for the changes exist
  • Runs in the swarm
  • Documentation reflects the changes
  • New module? Add your github username to .github/CODEOWNERS

@codecov
Copy link

codecov bot commented Mar 11, 2020

Codecov Report

Merging #1361 into master will increase coverage by 0.01%.
The diff coverage is 34.48%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1361      +/-   ##
==========================================
+ Coverage   72.61%   72.63%   +0.01%     
==========================================
  Files         201      201              
  Lines        8414     8426      +12     
  Branches      943      945       +2     
==========================================
+ Hits         6110     6120      +10     
+ Misses       2030     2029       -1     
- Partials      274      277       +3
Flag Coverage Δ
#integrationtests 57.56% <34.48%> (ø) ⬆️
#unittests 66.76% <3.44%> (-0.05%) ⬇️
Impacted Files Coverage Δ
...es/web/server/src/simcore_service_webserver/cli.py 66.66% <0%> (-1.34%) ⬇️
...r/src/simcore_service_webserver/computation_api.py 65.18% <43.47%> (-0.12%) ⬇️
.../director/src/simcore_service_director/producer.py 66.75% <0%> (-0.26%) ⬇️
...tor/src/simcore_service_director/registry_proxy.py 69.43% <0%> (+1.03%) ⬆️
...irector/src/simcore_service_director/exceptions.py 65% <0%> (+2.5%) ⬆️
...rc/simcore_service_director/registry_cache_task.py 97.43% <0%> (+2.56%) ⬆️

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 6ace13d...89c368c. Read the comment docs.

Pedro Crespo added 5 commits March 11, 2020 14:02
@pcrespov pcrespov force-pushed the enh/check-task-exists-in-db branch from 5c06420 to a223728 Compare March 11, 2020 13:02
@pcrespov pcrespov requested review from sanderegg, odeimaiz, ignapas and mguidon and removed request for sanderegg March 11, 2020 18:04
@pcrespov pcrespov self-assigned this Mar 11, 2020
@pcrespov pcrespov changed the title Enh/check task exists in db Check task exists in db and requirements upgrade Mar 11, 2020
Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

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

so it basically reduces the chance of the error happening but it might still happen right?

@pcrespov pcrespov changed the title Check task exists in db and requirements upgrade [enhancement] Check task exists in db and requirements upgrade Mar 12, 2020
Copy link
Member

@mguidon mguidon left a comment

Choose a reason for hiding this comment

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

Good to go. However, could you split "beautification" changes and "code" changes into different PRs?

@pcrespov pcrespov merged commit 87e6bf3 into ITISFoundation:master Mar 12, 2020
@pcrespov pcrespov deleted the enh/check-task-exists-in-db branch March 12, 2020 10:29
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.

4 participants