Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-16160

Fix constraint check in associate method

    XMLWordPrintable

Details

    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

            jbosch Jim Bosch added a comment -

            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.

            jbosch Jim Bosch added a comment - 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.

            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.

            salnikov Andy Salnikov added a comment - 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!

            salnikov Andy Salnikov added a comment - 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!
            jbosch Jim Bosch added a comment - - edited

            Looks good.  Better fix is DM-16221.

            jbosch Jim Bosch added a comment - - edited Looks good.  Better fix is DM-16221 .

            Thanks for review! Merged and done.

            salnikov Andy Salnikov added a comment - Thanks for review! Merged and done.

            People

              salnikov Andy Salnikov
              salnikov Andy Salnikov
              Jim Bosch
              Andy Salnikov, Jim Bosch, Vaikunth Thukral
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:

                Jenkins

                  No builds found.