-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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
chore: remove shadow write of new sip 68 dataset models #21986
chore: remove shadow write of new sip 68 dataset models #21986
Conversation
648f708
to
a0008fe
Compare
839ea7e
to
92a8d28
Compare
de7fb3c
to
ee16eca
Compare
ee16eca
to
880f9d3
Compare
880f9d3
to
3384e12
Compare
Codecov Report
@@ Coverage Diff @@
## master #21986 +/- ##
==========================================
- Coverage 66.95% 66.91% -0.05%
==========================================
Files 1807 1818 +11
Lines 69222 69471 +249
Branches 7403 7496 +93
==========================================
+ Hits 46351 46484 +133
- Misses 20966 21049 +83
- Partials 1905 1938 +33
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
3384e12
to
d2523c8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@betodealmeida definitely is more qualified of reviewing this than me, but I agree the dual-write is probably not ideal if we aren't going to resume working on the new models any time soon.
If it's something you are allow to share, can you elaborate what were the errors you saw?
@@ -35,6 +37,26 @@ class DatasetDAO(BaseDAO): # pylint: disable=too-many-public-methods | |||
model_cls = SqlaTable | |||
base_filter = DatasourceFilter | |||
|
|||
@classmethod | |||
def find_by_ids(cls, model_ids: Union[List[str], List[int]]) -> List[SqlaTable]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this related?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks unrelated... maybe this was supposed to be in another branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I thought I had mentioned it in the PR description, but yes tests were failing on dataset deletion due to sqlalchemy not having access to the database relationship in the session. I found other logs of this happening for this command, and fixed it here for the tests to pass. There was a query in the after update hook that I removed that was re-loading the database into the session as an unexpected side-effect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -35,6 +37,26 @@ class DatasetDAO(BaseDAO): # pylint: disable=too-many-public-methods | |||
model_cls = SqlaTable | |||
base_filter = DatasourceFilter | |||
|
|||
@classmethod | |||
def find_by_ids(cls, model_ids: Union[List[str], List[int]]) -> List[SqlaTable]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks unrelated... maybe this was supposed to be in another branch?
@betodealmeida and @eschutho what’s the status of SIP-68? Are there blockers for preventing us from reading from the new datamodel tables? |
@john-bodley after this change, the new datasets/models and the sqlaTable/columns, etc will be out of sync. Prior to this change, some of the permissions may have been out of sync as well. We're going to try this again, but maybe with a model by model approach so that we can maybe knock out some of the new models quicker and then do the dataset change last, which will be the most complex. We just haven't had much time to contribute to wrapping this up unfortunately. |
(cherry picked from commit 86d52fc)
SUMMARY
We're seeing some sqlalchemy errors related to the dual writes of the sip68 models, so I'm removing the dual writing so that we can decide whether to move forward again and do another backfill migration or start over smaller with one model at a time. We also haven't been writing permissions to the models, so we would need to run a migration for that in the future regardless of whether we stopped dual writing or not, so it seemed to make sense to take a step back and stop the errors for now.
I also fixed a test on dataset deletion due to sqlalchemy not having access to the database relationship in the session. The fix was to eagerly load the database with the dataset on lookup. I found other logs of this happening for this command, and fixed it here for the tests to pass. There was a query in the after update hook that I removed that was re-loading the database into the session as an unexpected side-effect.
TESTING INSTRUCTIONS
Normal dataset crud operations should work as normal.
ADDITIONAL INFORMATION