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

            Hide
            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.

            Show
            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.
            Hide
            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.

            Show
            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.
            Hide
            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!

            Show
            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!
            Hide
            jbosch Jim Bosch added a comment - - edited

            Looks good.  Better fix is DM-16221.

            Show
            jbosch Jim Bosch added a comment - - edited Looks good.  Better fix is DM-16221 .
            Hide
            salnikov Andy Salnikov added a comment -

            Thanks for review! Merged and done.

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

              People

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

                Dates

                Created:
                Updated:
                Resolved:

                  CI Builds

                  No builds found.