Skip to content
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

Adapt adapter to read directly with sql #2331

Draft
wants to merge 24 commits into
base: feature/relational-db
Choose a base branch
from

Conversation

luisa-beerboom
Copy link
Member

@luisa-beerboom luisa-beerboom commented Mar 22, 2024

For now this is just a copy of the backend processes and not yet attuned to the new db schema (it's also based on @jsangmeister 's backend-setup PR)

TODO:

  • Make get_many work with the db schema
  • Implement the other functions in read_adapter.py
  • Use the ReadAdapter in adapter.py
    • Remove unnecessary parameters from adapter.py payloads
  • What about locking? => for update behind every select query when fields are supposed to be locked
    • What about min/max/count/exists -> those can't use for_update => Will need further deliberation
    • Somehow test this locking mechanism => As discussed, will be done by @r-peschke
  • Test base class update to proper system
  • Don't just create a new transaction for every get call, use one passed down by the calling class
  • Use @retry_on_db_failure?

@luisa-beerboom luisa-beerboom added this to the 4.2 milestone Mar 22, 2024
@luisa-beerboom luisa-beerboom self-assigned this Mar 22, 2024
dev/entrypoint.sh Outdated Show resolved Hide resolved

class ReadAdapter:
connection: (
ConnectionHandler # TODO use PgConnectionHandlerService from datastore ?
Copy link
Member

Choose a reason for hiding this comment

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

This theme we should talk about with @jsangmeister. Perhaps we could easily use that from datastore, but the process/thread behavior is different.

Copy link
Member Author

@luisa-beerboom luisa-beerboom Apr 16, 2024

Choose a reason for hiding this comment

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

I think I am currently getting that connection handler anyway. As far as I understand it the datastore injector that I use to initialize this variable in ln 20 delivers the PgConnectionHandlerService when the ConnectionHandler is injected (as per this datastore init file).

Copy link
Member

@r-peschke r-peschke Apr 16, 2024

Choose a reason for hiding this comment

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

Yes, but you need the same connection and transaction from the pool.
But I expected the base class of the ReadAdapter should be the BaseDataStoreService and the goal is to realize the same functionality like the DatastoreAdapter class, just without calling the datastore, but read/write itself.

@luisa-beerboom luisa-beerboom force-pushed the 2311-adjust-adapter-read-methods branch from 886fa05 to 1ffa803 Compare April 16, 2024 10:48
@@ -11,6 +11,7 @@ pytest-cov==5.0.0
pytest-profiling==1.7.0
pyupgrade==3.15.2
pyyaml==6.0.1
psycopg[binary]==3.1.18 # only dev, in production use psycopg
Copy link
Member

Choose a reason for hiding this comment

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

May be should move this to the production requirement file, which should be used for all the common base packages.


def write_data(self, payloads: list[WritePayload]) -> None:
env = os.environ
connect_data = f"dbname='{env['DATABASE_NAME']}' user='{env['DATABASE_USER']}' host='{env['DATABASE_HOST']}' password='{env['PGPASSWORD']}'"
Copy link
Member

Choose a reason for hiding this comment

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

One connection should serve for the whole session, read and write together

@luisa-beerboom
Copy link
Member Author

luisa-beerboom commented Apr 24, 2024

@r-peschke This is absolutely not done yet (see list of points in description), but I have the basic functionality of the read_adapter done and I changed things to the extent where it is actually used in the proper Adapter now (although I haven't really tested that part yet).
Could you take a look at my code and see if I implemented the already implemented basic processes correctly?

@rrenkert rrenkert assigned rrenkert and unassigned r-peschke Oct 21, 2024
@Elblinator Elblinator modified the milestones: 4.2, 4.3 Dec 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants