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

[8.1] Add VO in Jobs table #7215

Merged

Conversation

simon-mazenoux
Copy link
Contributor

@simon-mazenoux simon-mazenoux commented Sep 22, 2023

See discussion #7197 for further information.

This will require DB changes.

BEGINRELEASENOTES

*WMS
CHANGE: add VO info in the table Jobs of JobDB

ENDRELEASENOTES

@simon-mazenoux simon-mazenoux changed the title feat: add VO in Jobs table [8.1] Add VO in Jobs table Sep 22, 2023
@@ -42,6 +42,7 @@ CREATE TABLE `Jobs` (
`JobName` VARCHAR(128) NOT NULL DEFAULT 'Unknown',
`Owner` VARCHAR(64) NOT NULL DEFAULT 'Unknown',
`OwnerGroup` VARCHAR(128) NOT NULL DEFAULT 'Unknown',
`VO` VARCHAR(32),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not:

Suggested change
`VO` VARCHAR(32),
`VO` VARCHAR(32) NOT NULL DEFAULT 'Unknown',

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By not having a default value, a query trying to insert a row without the VO field will raise an IntegrityError in sqlalchemy, which will quickly indicate to the developer that something is wrong. Besides, now that the VO is used to get information from the config and that this field can no longer be determined by the OwnerGroup, having values as "Unknown" could be problematic.

Copy link
Contributor

Choose a reason for hiding this comment

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

a query trying to insert a row without the VO field will raise an IntegrityError in sqlalchemy

Would it raise an IntegrityError if the column has no NOT NULL constraint?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you're right. I'll correct it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the end I ended up adding the DEFAULT 'Unknown' because the _update method in DIRAC actually translates to an UPSERT, which causes an error because of the insert statement:

2023-09-27T08:34:51,221683Z Framework/WorkloadManagement/JobDB DEBUG: _update: INSERT INTO Jobs (JobID, Status) VALUES (3, "Received") ON DUPLICATE KEY UPDATE Status=VALUES(Status)
2023-09-27T08:34:51,221956Z Framework/WorkloadManagement/JobDB ERROR: _update (INSERT INTO Jobs (JobID, Status) VALUES (3, "Received") ON DUPLICATE KEY UPDATE Status=VALUES(Status)): Execution failed. 1364: Field 'VO' doesn't have a default value

@simon-mazenoux simon-mazenoux force-pushed the feat-add-VO-in-Jobs-table branch 2 times, most recently from ae972ad to b1c6906 Compare September 27, 2023 06:44
@simon-mazenoux simon-mazenoux force-pushed the feat-add-VO-in-Jobs-table branch from b1c6906 to dde7b2b Compare September 27, 2023 08:57
@simon-mazenoux
Copy link
Contributor Author

@aldbr Would you mind also checking the changes I made to the wiki page ? (I don't know if you did it already)

@fstagni fstagni merged commit 60d00f8 into DIRACGrid:integration Oct 6, 2023
24 checks passed
@DIRACGridBot DIRACGridBot added the sweep:ignore Prevent sweeping from being ran for this PR label Oct 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sweep:ignore Prevent sweeping from being ran for this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants