-
Notifications
You must be signed in to change notification settings - Fork 24
feat(history): implement automatic history purge logic #992
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
base: main
Are you sure you want to change the base?
feat(history): implement automatic history purge logic #992
Conversation
|
@AshokThangavel - thank you again! @isc-kiyer @isc-dchui I'm curious if you think this meets the expectations from the conversation behind #977 - the ideal would be a scheduled task, but I think this approach is also valid. (Since you're not going to get history without running zpm commands.) Personally, I strongly believe we should maintain all history by default and have purge be opt-in (as this does). Most of the time it will be very lightweight unless there is a ton of activity on the system - and if there is a ton of activity, the history becomes more essential/valuable. For long-running production systems that may or may not have good change management procedures, the history could be a lifeline for understanding when/why something broke. |
isc-tleavitt
left a comment
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.
A few nits + needs a changelog. Also needs confirmation from other devs who were deeper in discussion on history - but I like this approach, FWIW.
src/cls/IPM/General/History.cls
Outdated
| } | ||
|
|
||
| /// Automatically purge old history based on retention settings | ||
| ClassMethod AutPurgeOldHistory() |
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.
nit: typo, should be AutoPurgeOldHistory
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.
Thank you!. I've fixed the typo in the method
src/cls/IPM/General/History.cls
Outdated
| quit | ||
| } | ||
| set retainDays = ##class(%IPM.Repo.UniversalSettings).GetHistoryRetain() | ||
| quit:(+retainDays <=0) |
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.
General code style comment:
We generally dislike postconditionals for code readability. I'd personally make an exception for:
- Loop exit conditions with simple tests (e.g.,
quit:key=""orquit:AtEnd) Write:verbose !,"Not worth putting this in it's own if block"
Adding this to my list of things to formalize in https://github.com/intersystems/ipm/blob/main/CONTRIBUTING.md.
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.
Thank you!. I've modified the post condition
|
Hi @isc-tleavitt I also completely agree with your point on making this opt-in. It ensured the default behavior remains unchanged so history is preserved unless a user explicitly configures |
Feature: Implement Automatic History Purge
Overview
This PR introduces an automated history retention policy in IPM. It allows users to configure how many days of history should be kept, preventing the history table from growing.
fixes #977
Key Changes
HistoryRetainto the IPM configuration.config set HistoryRetain 10: Retains the last 10 days of history.config set HistoryRetain 0: Disables the auto-purge.Implemented
AutoPurgeOldHistory. This method checks for and removes records older than the configured threshold.CLI Usage
Users can manage the retention policy directly via the IPM shell:
zpm:USER> config set HistoryRetain 10zpm:USER> config get HistoryRetainzpm:USER> config set HistoryRetain 0Testing & Validation
Unit tests have been implemented to verify the full lifecycle of the configuration and the purge engine: