Add /api/docs/:docId/wipeCache endpoint #1486
Conversation
|
If we agree such an endpoint could be useful, the next step will be to implement test for them. |
This endpoint is the corollary of .../flush: it discards any local copy (the cache) so the doc worker is forced to fetch the document and metadata from the remote storage. Especially useful when changes has been brought to the remote doc storage and we want to force the doc worker to fetch the version from there. WARNING: it discard any changes that had not been pushed yet.
fflorent
force-pushed
the
wipe-cache-endpoint
branch
from
ea5f6de to
9a7f3b8
Compare
| // Especially useful when changes has been brought to the remote doc storage | ||
| // and we want to force the doc worker to fetch the version from there. | ||
| // WARNING: it discard any changes that had not been pushed yet. | ||
| this._app.post('/api/docs/:docId/wipeCache', canEdit, throttled(async (req, res) => { |
There was a problem hiding this comment.
| this._app.post('/api/docs/:docId/wipeCache', canEdit, throttled(async (req, res) => { | |
| this._app.post('/api/docs/:docId/wipeCache', canOwner, throttled(async (req, res) => { |
| @@ -234,6 +234,10 @@ export class DocStorageManager implements IDocStorageManager { | |||
| // nothing to do | |||
| } | |||
|
|
|||
| public async wipeCache(): Promise<void> { | |||
| // nothing to do | |||
There was a problem hiding this comment.
Maybe this should throw an error saying it would be a bad idea to wipe a "cache" if it is actually the only copy :)
There was a problem hiding this comment.
I'm also thinking that checking that a version of the doc exists in S3 could sometimes be life-saving :
- you're working on a document
- S3 storage is down, but you can still work on your document (in the background, snapshots don't trigger though)
- you wipe the "cache"
- all of your work is gone
There was a problem hiding this comment.
Maybe this should throw an error saying it would be a bad idea to wipe a "cache" if it is actually the only copy :)
I am not very comfortable with raising an exception, here are the reasons:
- A cache may be many things, hence I feel like it is acceptable to just "do nothing" (just like the
flushmethod) ; - this function could be reused elsewhere (I especially think of Remove document cache after a certain inactivity #1494), raising an exception would mean that the caller is aware of the nature of the storage management (local, S3, whatever), the inheritance is convenient for just allowing to ignore the call ;
An alternative I propose is otherwise that the wipeCache method is optional in the IDocStorageManager interface. The caller must check whether the method exists: use optional chaining or a condition + raise an exception if the method is not implemented (something that the endpoint could do).
What do you think?
|
No objection to the functionality. Some bike-shedding opinions about the name. Cache is ambiguous. Also I'm a little sad about the divergence that has crept in of endpoint naming e.g. "/force-reload" vs "removeUnused" but I think the latter style is winning so we should converge on that. But what do you think about making this functionality be an option on "/force-reload", like "?full=true" or "?clearCache=true" (I think cache is less ambiguous in the context of a document reload). |
I agree that this would be great. Maybe even adding it to the UI in a later PR ? |
Context
See #1485
Proposed solution
Add an /api/docs/:docId/wipeCache endpoint
This endpoint is the corollary of .../flush: it discards any local copy (the cache) so
⚠️ it discard any changes that had not been pushed yet.
the doc worker is forced to fetch the document and metadata from the remote storage.
This way, when a client open the document again, it is forced to fetch it from the remote storage.
Related issues
Fixes #1485
Has this been tested?