The Wayback Machine - https://web.archive.org/web/20260612125212/https://github.com/kedro-org/kedro-viz/pull/694
Skip to content

Refactor repositories & database access to prepare for subscription#694

Merged
limdauto merged 33 commits into
mainfrom
feat/breakup-repositories
Jan 19, 2022
Merged

Refactor repositories & database access to prepare for subscription#694
limdauto merged 33 commits into
mainfrom
feat/breakup-repositories

Conversation

@limdauto

@limdauto limdauto commented Jan 13, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

This PR:

  • Completely abstract database logic inside repositories within the data access layer and hide it from the API layer.
  • Change from 1 long running database session per server process to one session per query through SQLAlchemy's session mechanism for better isolation.
  • Break up the current repositories.py into multiple modules, each containing one repository for easier maintanence.
  • Break up the repositories tests accordingly.
  • Refactor and improve GraphQL tests, breaking them up into different test modules for queries, mutations for schema.
  • Fix a couple of small undetected bugs around notes & title handling.

Development notes

QA notes

Checklist

  • Read the contributing guidelines
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added new entries to the RELEASE.md file
  • Added tests to cover my changes
@limdauto limdauto changed the title [WIP] Refactor repositories to prepare for subscription Jan 18, 2022
@limdauto limdauto changed the title Refactor repositories to prepare for subscription Jan 18, 2022
@limdauto limdauto requested a review from antonymilne January 18, 2022 00:41
@limdauto limdauto marked this pull request as ready for review January 18, 2022 00:53
limdauto
limdauto commented Jan 18, 2022
View reviewed changes
Comment thread package/kedro_viz/api/graphql.py
Comment thread package/tests/conftest.py
Comment thread package/tests/test_api/test_graphql.py
antonymilne
antonymilne approved these changes Jan 18, 2022
View reviewed changes

@antonymilne antonymilne left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks amazing, great set of changes! ⭐ I've left lots of questions just for my own understanding mainly.

Please could you remind me again why we want to change from one long running db session per server process to one session per query? Was it so that an update query can't interfere with a read one? Is there any performance hit due to the need to connect/disconnect from the db multiple times?
Comment thread demo-project/src/demo_project/hooks.py Outdated
Comment thread package/kedro_viz/api/graphql.py Outdated
Comment thread package/kedro_viz/api/graphql.py
Comment thread package/kedro_viz/api/graphql.py Outdated
Comment thread package/kedro_viz/api/graphql.py
Comment thread package/kedro_viz/data_access/managers.py
Comment thread package/kedro_viz/data_access/repositories/runs.py
Comment thread package/kedro_viz/data_access/repositories/runs.py
def create_or_update_user_run_details(
self, updated_user_run_details: Dict
) -> Optional[UserRunDetailsModel]:
with self._db_session_class.begin() as session: # type: ignore

@antonymilne antonymilne Jan 18, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there really no way in sqlalchemy to do an upsert more simply than this? 🤔 Seems odd.

@limdauto limdauto Jan 18, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yea I forgot why it was did this way. Let me double check

@limdauto limdauto Jan 19, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Right, I remember now. SQLAlchemy has support for ON CONFLICT (although it's quite involved too), but only for the engine that supports it. SQLite doesn't: https://www.sqlite.org/lang_conflict.html, so we look before we leap :) The setattr could potentially be more readable if we spell out the properties. Let me do that.

@limdauto limdauto Jan 19, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@AntonyMilneQB I have spelled out the properties and it's indeed more readable.
Comment thread package/kedro_viz/integrations/kedro/sqlite_store.py
Comment thread package/tests/test_api/test_graphql/test_mutations.py
rashidakanchwala
rashidakanchwala approved these changes Jan 19, 2022
View reviewed changes

@rashidakanchwala rashidakanchwala left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM! It work's well -- haven't come across any issues while testing.

I am still reviewing the PR from a learning purpose.
limdauto and others added 2 commits January 19, 2022 13:41
@limdauto @antonymilne
Update package/kedro_viz/api/graphql.py
7f1489d 
Co-authored-by: Antony Milne <49395058+AntonyMilneQB@users.noreply.github.com>
@limdauto
Fix PR comments
1054f6d
@limdauto limdauto force-pushed the feat/breakup-repositories branch from a949660 to 1054f6d Compare January 19, 2022 14:29
@limdauto limdauto merged commit f49d174 into main Jan 19, 2022
@limdauto limdauto deleted the feat/breakup-repositories branch January 19, 2022 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants

@limdauto @rashidakanchwala @antonymilne