-
Notifications
You must be signed in to change notification settings - Fork 51
Record transfer limit for data refresh #474
Conversation
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.
LGTM, except a note for the future.
copy_data = get_copy_data_query(table, shared_cols, approach=approach) | ||
environment = config("ENVIRONMENT", default="local").lower() | ||
limit_default = 100_000 | ||
if environment in {"prod", "production"}: |
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.
We should settle on one of these in the future.
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.
Agreed, just trying to cover all my bases 😅 We've had some discussion about local
as well in #475, I'll make an issue for unifying this everywhere.
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.
Again, not the biggest 🐍 python authority but the code makes sense and I don't see any issues. Nice!
So I ran into that issue where it hangs on the cleaning step again, and I was able to resolve it by removing the garbage collection steps 🙃 I'll have to do a deep dive on that at some point |
Fixes
Fixes #472 by @AetherUnbound
Description
This PR adds a new variable,
DATA_REFRESH_LIMIT
, and some additional logic for interpreting an appropriate limit based on the environment the ingestion server is deployed in. Inprod
orproduction
, the default limit is removed and all records are copied. In any other environment, at most 100k records are copied by default (although more or fewer can be specified using the aforementioned environment variable).I also split the unit tests up and added tests for the altered query. This was a huge pain, because trying to convert the
psycopg2.SQL
objects into strings requires a database connection which is way too much overhead for such a simple test.Testing Instructions
just ing-testlocal
Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin