I think allowing concurrent DB access is not enough - we need to make sure package pool and published repositories are safe to be used concurrently (which is not the case today), so DB lock protects today implicitly package pool/published repos.

Yes this is not enough. Our approach to this has actually developed over time. Now we do a locking on a task basis and each task can define what resources it requires.

I haven't gotten to update the PR... will do so when I find time but if you wanna have a look see master branch of our fork:
https://github.com/adfinis-forks/aptly/

That sounds interesting, and I think locking on per-resource level might be the way to go here...

@smira
I have finally gotten around to update this PR. I have updated the description to better explain what this PR does.

In general are there two core concepts:

1. locking per resource
2. task api for async job support

Let me know what you think.

Thanks, I'll schedule it for post-1.1.0 as this looks scary enough for planned June release (already a lot of changes queued).

I agree properly better to postpone it and polish it a bit more. What I want to note is though that we already use this code in production and it proved to be stable.
If you have any comments on the implementation let me know so I can have a look.

@smira I have updated PR so it should cleanly apply to master again. As version 1.1.0 is now released it would be great if you could comment on this PR what you think and have we shall move forward to possible integrate into the next version. I am happy to adjust the code as needed.

I love the idea.

Considering this changes return values of existing endpoints I'd say this needs some compatibility adjustments.

Specifically, everything should get an additional new route /api/v2/..., the existing routes in /api/... internally call the new v2 handlers but instead of returning the task they wait for it to finish and return the result. Meaning POST /api/repos/:name/snapshots continues to behave the way it does right now in production. When sending to the new v2 endpoints POST /api/v2/repos/:name/snapshots you get the task id instead. So, old endpoints stay the same, new v2 endpoints change to tasks.

Food for thought: I am wondering if a websocket isn't the more "modern" and flexible approach to this. i.e. instead of getting a taskid the client gets a websocket URI, then the client connects to the socket to get task updates from it. Calling /wait with a super long HTTP GET timeout seems somewhat underwhelming. Also, with a socket it'd probably be very easy to implement "streaming" of the output log of the task on the client side. Whereas right now you either have to GET /output numerous times while the task is running (getting mostly the same data over and over again), or have no output until /wait returns and get it all at once.

Taking it a step further, I could imagine this enabling us to put the CLI ontop of the API, so you could actually operate a remote aptly instance via your local CLI! Well, one can dream 😸

@apachelogger Totally agree that we should make this change in a backwards compatible fashion and the way you outlined should be straight forward.

I love the idea of web sockets... I think though we should make using web sockets on the api only an additional feature and not required per default though so simple REST client script are still possible.

This way we can also get the async api merged and add web sockets in a future release (which would then also let the CLI use the API).

Codecov Report

Merging #459 into master will increase coverage by 0.3%.
The diff coverage is 65.66%.

@@            Coverage Diff            @@
##           master     #459     +/-   ##
=========================================
+ Coverage   60.64%   60.94%   +0.3%     
=========================================
  Files          50       54      +4     
  Lines        6276     6422    +146     
=========================================
+ Hits         3806     3914    +108     
- Misses       2033     2081     +48     
+ Partials      437      427     -10

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9a704de...52d4336. Read the comment docs.

I wonder if it could be HTTP/2 Push events in the end.

I want to look into this more, but I don't see why exactly we can't make async return optional? So that sync/async options are both available. In the end in the local network environment, there's nothing unusual about setting HTTP response timeout to say 10 minutes and waiting for API completion in one call. Long polling, async response makes more sense when API is not local and we can't control timeouts on the way to aptly service endpoint.

I would like to try to see if it's possible to split into a smaller PRs and commit them one by one, with thorough review for each one.