feat: Clean cache command#1394
Conversation
83f2262 to
d52c4ad
Compare
7c59782 to
444977d
Compare
c2dc7b8 to
1041695
Compare
dc31834 to
f5def12
Compare
1041695 to
66296d5
Compare
77f7320 to
aa280da
Compare
34fd55f to
7473bd4
Compare
7b17cc0 to
569ff71
Compare
569ff71 to
36fd376
Compare
| @@ -0,0 +1,80 @@ | |||
| import path from "node:path"; | |||
There was a problem hiding this comment.
It seems odd that this file is placed next to the other files responsible for managing the framework packages but doesn't use any of them. I would expect better integration here, e.g. no hardcoded assumptions like the framework/ directory name as well as checks for existing lockfiles to prevent deleting files while a download is running
There was a problem hiding this comment.
I understand your point, but what botters me is that this might need a bit more of a refactoring.
Here's my rationale.
The AbstractInstaller is an abstract class that implements the lock logic. As the name suggests- it's an installer that is extended by the npm and maven installers.
Cache clanup, except from having in common the locks and paths, is a completelly different topic- it needs to clean the framework files. I have also seen that the framework folder is not configured in the AbstractInstaller, but is hardcoded in every installer.
The only clean option I forsee is to consolidate and reuse the locking logic and abstract the "framework" dir within the AbstractInstaller. Then somehow reuse this information in the cache cleaner.
There was a problem hiding this comment.
I have refactored the code, so that framework folder is reused accross classes and the locking is respecte.
Hopefully, this change addresses you comment: 8a53eb8
Let me know if you have other concerns on that matter
There was a problem hiding this comment.
Great. Just to bring this up: We now check that there is no active lock before deleting files but we do not set a lock during the deletion
There was a problem hiding this comment.
Thanks! I have missed that! Now, should be fine: f6e0404
There was a problem hiding this comment.
What do we need the new manifest-*lock for?
There was a problem hiding this comment.
Yep, these are missleading. I have changed them! Thanks!
There was a problem hiding this comment.
Now they are renamed, but I'm still wondering what they are for 😅
I.e. what is their purpose compared to the locks we already have.
There was a problem hiding this comment.
Ah, that’s fair 😅
Generally, they solve the issue with the race conditions.
What you have described above is just one if the cases! There are more.
And these locks solve exactly that.
So far, we had only one lock that was for “downloading” the artifacts. But in there’s also an Install phase that actually copies the files to the cache folder!
We have maven and npm install. I just wanted to distinguish them as they use different origins. Otherwise, they could easily be with the same name (I think so 🤔)
From that perspective, there were lots of options for a race conditions window:
- between cache clean and the confirmation window
- Between download and install phase of the packages
Before the latest change, at certain point, both could have continued.
Now, (all) locks are respected and even if there’s a little chance to jump between the phases, it will notice the lock for the other process and stop.
Hopefully, this makes it a bit clearer!
If you insist, I can make the locks with the same name/prefix i.e. ‘instal-*.lock’
There was a problem hiding this comment.
I can't find this "phase that actually copies the files to the cache folder". Let's take a look together in a call
|
I think the usability of the command could be improved.
|
|
I have tried to improve the situation with the infmration for the execution of the Let me know if there's something more you'd expect. Regarding the size report, there are some considerations we need to be aware of:
I have tried to somehow get advantage of these findings and provide the optimal solution- show metrics for what is fast and provide generic messages. Any other solutions will require certain compromises. |
EditNow the UX of this command is revised! The real issue is enormous cache! Collecting full information about what would be deleted is a haevy work and we simply cannot do it real time. In the end, for the end user it's important what would be deleted! ConfirmationChecking cache at /my/data/dir/.ui5 …
The following cached data will be removed:
• UI5 Framework packages /my/data/dir/.ui5/framework (1 project, 18 libraries, 212 versions)
• Build cache (DB) /my/data/dir/.ui5/buildCache/v0_7 (100.0 KB)
Do you want to continue? (y/N)During cache cleanupIf we now decide to accept purging of the hughe cache, it will take some time cleaning it up. We cannot do anythinbg about it, but simply wait for the fuiles to be deleted. ⠏ UI5 Framework packages …/sap.m/1.52.14/src/sap/m/BarRenderer.js 2.86 sFinal summary✓ Removed UI5 Framework packages (/my/data/dir/.ui5/framework · 1 project, 18 libraries, 212 versions)
✓ Removed Build cache (DB) (/my/data/dir/.ui5/buildCache/v0_7 · 100.0 MB)
Success: Cleaned UI5 Framework packages and Build cache (DB)Hopefully, this is good enough as UX and fast enough, so that developers get feedback what is actually happenning |
|
That's better. On my system it's very fast now: But I think the term I don't really think we need a progress indicator for the deletion process. It is expected to take a while, I would rather not add code just for that. |
5863e5f to
3d761f2
Compare
Provide common interface for cache cleanup, but distribute the real cleanup among the respective destinations
3f0d21c to
4d39800
Compare
The command completely cleans the cache by removing the cache files as well as cleaning up the SQLite records.
It does not wipe out the SQLite DB file(s)
JIRA: CPOUI5FOUNDATION-891