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

Fix machine_type unique constraint to support a machine_type for multiple sites #220

Merged
merged 5 commits into from
Dec 2, 2021

Conversation

giffels
Copy link
Member

@giffels giffels commented Nov 25, 2021

This pull request fixes a bug in the database schema of the SqliteRegistry plugin. In the current schema it was not possible to have the very same MachineTypes on different Sites, since the unique constraints put on the database table were too strict. This pull request introduces a more relaxed unique constraint, that requires only the combination (MachineType, Site) to be unique.

In addition, this pull request introduces a more relaxed unique constraint on the Resources table, too. Now it is possible that the same remote_resource_uuid can be used at different sites within the same table. This was not mentioned in #219, but could lead to further trouble in the future. For example, two HTCondor batch systems at different sites can have the same job number used as remote_resource_uuid.

In addition, the unittest of the SqliteRegistry have been improved by adding both scenarios mentioned above and de-duplicate some of the test code.

Fixes #219

@giffels giffels added the bug Something isn't working label Nov 25, 2021
@giffels giffels added this to the 0.7.0 - Release milestone Nov 25, 2021
@codecov-commenter
Copy link

codecov-commenter commented Nov 29, 2021

Codecov Report

Merging #220 (62ad6d5) into master (c532a19) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #220   +/-   ##
=======================================
  Coverage   99.55%   99.56%           
=======================================
  Files          41       41           
  Lines        1810     1823   +13     
=======================================
+ Hits         1802     1815   +13     
  Misses          8        8           
Impacted Files Coverage Δ
tardis/plugins/sqliteregistry.py 100.00% <100.00%> (ø)

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 c532a19...62ad6d5. Read the comment docs.

@giffels giffels added the potential problem Avoid foreseeable misuse label Nov 29, 2021
@giffels giffels marked this pull request as ready for review November 30, 2021 10:14
@giffels giffels requested review from a team, maxfischer2781 and HerrHorizontal and removed request for a team November 30, 2021 10:21
Copy link
Member

@maxfischer2781 maxfischer2781 left a comment

Choose a reason for hiding this comment

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

Thanks for catching and fixing both issues!

I have some comments/requests on code health but nothing major.

tardis/plugins/sqliteregistry.py Outdated Show resolved Hide resolved
tardis/plugins/sqliteregistry.py Outdated Show resolved Hide resolved
tardis/plugins/sqliteregistry.py Outdated Show resolved Hide resolved
tardis/plugins/sqliteregistry.py Outdated Show resolved Hide resolved
tests/plugins_t/test_sqliteregistry.py Show resolved Hide resolved
@maxfischer2781
Copy link
Member

By the way, my SQL is quite a bit rusty. I think the INSERT OR ROLLBACK is fine but I'm not sure if it's actually needed for site/machinetype, seeing how you check these before attempting the insert.

HerrHorizontal
HerrHorizontal previously approved these changes Nov 30, 2021
@giffels
Copy link
Member Author

giffels commented Nov 30, 2021

By the way, my SQL is quite a bit rusty. I think the INSERT OR ROLLBACK is fine but I'm not sure if it's actually needed for site/machinetype, seeing how you check these before attempting the insert.

Actually, it is not needed. The code is only called when the Site or machine_type is not present in the DB. However, in case of any unexpected failure ROLLBACK will throw an exception after the DB rollback. This is an improvement to the IGNORE which was used before, that just silently ignored the errors.

Copy link
Member

@maxfischer2781 maxfischer2781 left a comment

Choose a reason for hiding this comment

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

INSERT INTO tardis

@giffels giffels merged commit d66c293 into MatterMiners:master Dec 2, 2021
@giffels giffels deleted the fix-db-schema branch December 2, 2021 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working potential problem Avoid foreseeable misuse
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Problem with MachineType name at several sites
4 participants