Details
-
Type:
Bug
-
Status: Done
-
Resolution: Done
-
Fix Version/s: None
-
Component/s: daf_butler
-
Labels:
-
Story Points:1
-
Epic Link:
-
Sprint:BG3_F18_10
-
Team:Data Access and Database
Description
My first attempt to implement constraint check in associate() method (DM-15686) apparently broke everything (DM-16159). Looks like code in obs_subaru depends on uniqueness check to be done in addDataset() method. My commit moved that check to associate() which is apparently too late, tha fix for that it to leave the test in addDataset(). IT would be interesting to also understand what is going on in obs_subaru because in the future we'll probably switch to table-level constraints and that can break things again.
Attachments
Issue Links
Activity
Replying to Jim's comment above - in this case SQLAlchemy does not start a rollback, RawIngestTask does its own transaction management and it asks registry not to do any transaction management by passing transactional=False to addDataset. RawIngestTask catches ValueError though (may not be the best exception type in this case as ValueError could happen for some other reasons) and tries to do smatr things then but assumption (in unit test at least) is that registry is not modified in that case.
If we move to checks based on database constraints the situation may become more complicated. Some checks/failures could only happen when transaction is committed, meaning that addDataset() won't be able to detect anything or raise its exception, and RawIngestTask will need to do something when it tries to commit its "large" transaction. In that respect I think having one large transaction is even more problematic, it easier to handle shorter transaction which could fail than one huge transaction.
Next attempt is here, this time with Jenkins (centos builds finished long time ago, macox is still in progress, but obs_subaru has passed)! There are two commits on this PR, one is identical to my yesterday's and second applies two small fixes on top of that.
And I am terribly sorry for yesterday screw up!
Thanks for review! Merged and done.
I agree 100% with all of that (both diagnosis and workaround). I suspect we will need to start using explicit ON CONFLICT instructions or something, or perhaps scale back what we think we can promise in terms of guarantees in ingest.