-
Notifications
You must be signed in to change notification settings - Fork 82
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
RHINENG-4700_and_5552: Use deletion for culling and rename attributes in custom staleness #1573
RHINENG-4700_and_5552: Use deletion for culling and rename attributes in custom staleness #1573
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.
How this will work with the current databases? I saw the changes on migrations folder only for the fields name creation..,
After making these changes, I successfully ran the make task |
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
Did you run the DB upgrade on an already existing database or on a new one? This change to the DB migration is my only concern with this PR, I think the proper way would be to create a new migration instead of updating an existing one. I don't know if this approach will work on existing databases in Stage and Prod. But I've never done this, so correct me if I'm wrong. Otherwise, the PR looks good. |
Thank you @fstavela; very good point. I ran it on a new DB but let me run against an older one, i.e. migration. |
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.
+1 to the point about the migrations - you'll need to make a new migration rather than modify the existing ones.
Also, there are a few places where the field names haven't been updated, and it would make sense to update them all at once.
api/staleness_query.py
Outdated
"conventional_staleness_delta": config.conventional_staleness_seconds, | ||
"conventional_stale_warning_delta": config.conventional_stale_warning_seconds, | ||
"conventional_culling_delta": config.conventional_culling_seconds, | ||
"immutable_staleness_delta": config.immutable_staleness_seconds, | ||
"immutable_stale_warning_delta": config.immutable_stale_warning_seconds, | ||
"immutable_culling_delta": config.immutable_culling_seconds, | ||
"conventional_time_to_stale": config.conventional_staleness_seconds, | ||
"conventional_time_to_stale_warning": config.conventional_stale_warning_seconds, | ||
"conventional_time_to_delete": config.conventional_culling_seconds, | ||
"immutable_time_to_stale": config.immutable_staleness_seconds, | ||
"immutable_time_to_stale_warning": config.immutable_stale_warning_seconds, | ||
"immutable_time_to_delete": config.immutable_culling_seconds, |
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 might as well update the field names in the config file and environment variables too; it'll be confusing if we don't.
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.
Done!!!
hmmm... I did bulk find and resplace. Let me look. Thanks. |
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!
Thanks for this PR.
the _time_to
was a good approach to solve the variables naming we discussed!!
Overview
This PR is being created to address RHINENG-4700 and RHINENG-5552 and replace the PR #1550
PR Checklist
Secure Coding Practices Documentation Reference
You can find documentation on this checklist here.
Secure Coding Checklist