Purging of local datasets #1103
Conversation
| ## from the local node. Does nothing and returns 200 | ||
| ## if the dataset is not locally available. | ||
| ## | ||
| var headers = buildCorsHeaders("DELETE", allowedOrigin) |
There was a problem hiding this comment.
I don't think you need to take care of the CORS here. This is the purpose of the endpoint that you created before with MethodOptions. When a preflight request is sent, it will hit the method 'OPTIONS', and you allowed 'GET' and 'DELETE' methods. So the preflight request will be successful, and the 'DELETE' request will be executed.
There was a problem hiding this comment.
Yeah OK. I thought you always had to check the CORS origin, even if you have a preflight, cause callers can still not use the preflight, or otherwise screw things up.
There was a problem hiding this comment.
OK, so I removed some of the CORS handling, but left that line cause other endpoints include it. This does smell fishy though: if we are to follow the fetch standard, it seems we're sending quite a few headers we don't need to in a bunch of places?
There was a problem hiding this comment.
You could remove this line as well actually. The other endpoints include it because if there is an error, you still need to pass the CORS headers, yes. But in your case, your created a specific endpoint to handle this so you don't need to pass the headers even if there is an error.
If think the only header we don't need is X-Requested-With. But Access-Control-Allow-Origin and Access-Control-Allow-Methods are definitely needed. Access-Control-Max-Age is for caching the previous header.
There was a problem hiding this comment.
OK so if we're to follow the standard, my understanding is that I should always send Access-Control-Allow-Origin, even in this endpoint.
Then, for the preflight endpoint, I should send:
Access-Control-Allow-Origin;Access-Control-Allow-Methods;Access-Control-Max-Age;Access-Control-Headers.
There was a problem hiding this comment.
Not sure why we need Access-Control-Allow-Origin in the endpoint itself.
There was a problem hiding this comment.
But yes you are right we need it
|
I think I've addressed your comments @2-towns, either by fixing them or by continuing the discussion. |
| without manifest =? Manifest.decode(manifestBlock), err: | ||
| return failure(err) | ||
|
|
||
| for i in 0 ..< manifest.blocksCount: |
There was a problem hiding this comment.
This will very likely lockup the node for the duration of the delete, unless we yield some time back to the main thread. I think there is an IdleAsync method in chronos, that will schedule this after all the IO related work is done, otherwise, we can always do an async sleep after some number of blocks have been processed.
There was a problem hiding this comment.
Yeah good catch - I forget that those are actually blocking all the way. I'll add one of those.
There was a problem hiding this comment.
OK I've added a fix for this, I ended up going with a time-based quota as I think that's a lot more predictable than counting iterations. It also ended up being simple to implement.
| @@ -4,7 +4,6 @@ import std/os except commandLineParams | |||
|
|
|||
There was a problem hiding this comment.
Looks like I've ran nph on build.nims as part of some commit, for some reason, in this branch. Rolling this back would be more trouble than just leaving it here, so I'll "bundle" it with this PR.
This PR adds an endpoint for deleting local datasets by their CID. Consistently with download API, passing a block CID will cause a block to be deleted, whereas passing a manifest CID will cause all the blocks under the dataset represented by that manifest to be deleted.