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

refactor: remove non necessary SQL queries #227

Open
wants to merge 4 commits into
base: unavailable-resources-management
Choose a base branch
from

Conversation

bolinocroustibat
Copy link
Contributor

@bolinocroustibat bolinocroustibat commented Nov 22, 2024

Needs to be merged after #163.

This is a refactoring PR to remove many non necessaries SQL queries, using the existing data in the code instead of re-querying it.

@bolinocroustibat bolinocroustibat self-assigned this Nov 22, 2024
@bolinocroustibat bolinocroustibat changed the title refactor: remove useless queries refactor: remove non necessary SQL queries Nov 26, 2024
Copy link
Contributor

@magopian magopian left a comment

Choose a reason for hiding this comment

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

Nice cleanup, always feels good to remove some DB queries <3
🚢

"""Notify udata of the result of a parsing"""
# Get the check again to get its updated data
Copy link
Contributor

Choose a reason for hiding this comment

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

Did the data get updated in the meantime? Is that why it was re-querying the data?
I struggle to see how the data could be meaningfully changed in this time period, which I assume is very short, unless this code is long-running?

Copy link
Contributor Author

@bolinocroustibat bolinocroustibat Nov 27, 2024

Choose a reason for hiding this comment

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

I remember stumbling on a Sentry error case where the check seems that it was updated in the meantime, so I added this comment a month ago, but I asked myself the exact same question, I couldn't see why the check should have been changed within the period.
In this PR, each Check.update() execution now also return the updated check record and replace the object, so we now should be 100% sure the object is updated before it is passed to notify_udata in the finally clause.
Will be tested on dev and demo.

udata_hydra/db/resource.py Show resolved Hide resolved
udata_hydra/db/resource.py Show resolved Hide resolved
udata_hydra/crawl/preprocess_check_data.py Show resolved Hide resolved
@bolinocroustibat bolinocroustibat force-pushed the unavailable-resources-management branch 2 times, most recently from df50c84 to 5a914c6 Compare November 27, 2024 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants