Job queue, matrix builder, concurrency control #709
Conversation
|
Main chunks of work to be done at this point:
|
|
But the first two bullets can be implemented as separate changes, right? They're not needed for strict parity. |
|
@fsoikin the overall goal is to merge #669 ASAP, so that we can reupload the whole registry. We could be doing all these changes in separate PRs but I don't see the point since they will all need to end up in the |
app/src/App/Server/Router.purs
Outdated
| insertPackageJob :: PackageOperation -> ContT Response (Run _) Response | ||
| insertPackageJob operation = do | ||
| lift $ Log.info $ "Enqueuing job for package " <> PackageName.print (Operation.packageName operation) | ||
| jobId <- newJobId | ||
| lift $ Db.insertPackageJob { jobId, payload: operation } | ||
| jsonOk V1.jobCreatedResponseCodec { jobId } |
There was a problem hiding this comment.
The old code checked for running jobs before creating new ones:
lift (Db.runningJobForPackage packageName) >>= case _ of
Right { jobId, jobType: runningJobType } -> ...Looks like the duplicate checking is no longer there, is that intentional?
There was a problem hiding this comment.
I have noticed as well, and I am reshuffling this part of the code to add this back, among other things.
I have been thinking about splitting the "package job" table as well into different tables for publish/unpublish/transfer, because they do require different checking (publishing/unpublishing needs to check for the tuple package name and version, transferring only needs to match the package name). Thoughts?
app/src/App/SQLite.js
Outdated
| JOIN ${JOB_INFO_TABLE} info ON job.jobId = info.jobId | ||
| WHERE info.finishedAt IS NULL | ||
| AND info.startedAt IS NULL | ||
| ORDER BY info.createdAt DESC |
There was a problem hiding this comment.
Why DESC here? Don't we want the oldest job to go out first (FIFO), like it previously was? Same for the other selectNext* functions.
There was a problem hiding this comment.
Yeah I have noticed too, it should be ASC.
And it should probably not be LIMIT 1 either, as we likely want to do some toposorting on the result. I have not figured out yet where we draw the line though, I wouldn't like to be toposorting thousands of matrix jobs over and over, so we might want keep the limit and toposort on insertion. But then we need to ORDER BY something else than creation date when fishing that out. Ideas?
|
Also the tests are failing because of a missing |
In addition to your list (some responses to that below), the other point that I'm not seeing here is that we need to ensure that the github issue module only proxies requests over to the server API and writes comments on 'notify' logs but does not actually execute e.g. the publish operation itself. Otherwise we've defeated the purpose of the pull request, as we can't enforce a lock on the git commits anymore.
This makes sense to me: start with no-dependency packages, and on completion we queue dependents that are satisfied by this package existing, propagate outwards.
What do we need to do beyond what we have today? Today, we open an issue, The same approach is used for transfers or unpublishing: open the issue as a |
Yeah, I have not fixed that yet because I was not sure we need the version in there at all. Help me with reminding why we need the version now 😄 Also: I see these are on a different branch? I will cherry-pick them here, and feel free to push new stuff here as well.
Yeah it's planned but I want to thread through the matrix jobs and the package set jobs to the database before going to the github interaction part. It might be a nice spot where we can parallelise work if you want to have a go at it. I will push the current state of my branch so you have the latest.
Yeah if you think it's straightforward then it probably is 😄 I have not looked at this chunk of code yet so I might be overthinking stuff. I like the public aspect of the package set updates as well, and I in fact want to make it public-only, in the sense that I'd put the package-set server endpoints behind encryption/authentication (with just a simple shared secret for example) so that only the code that runs on github can hit them and we don't have to worry about things coming to the registry server from other places. No strong feelings about this though if you think it's too convoluted |
|
Ah, if you want it to be forced public then we could indeed do a shared secret we send in the HTTP header or something. I’d like transfer and unpublish to be public too — we could potentially remove the ability for pacchettibotti signatures to be valid for those and only the shared secret can be used as a trustee override? |
the publishCodec requires a version file but the test fixtures weren't updated to include it
The JS insertMatrixJobImpl expects columns [jobId, packageName, packageVersion, compilerVersion, payload] but the PureScript types were missing packageName and packageVersion
@thomashoneyman we can't make unpublish and transfer public-only: package maintainers should be able to do it at the CLI (with Spago or whatever else), with their keys and so on. I am interested in making public-only the package-sets endpoint, but maybe that's unnecessarily extending the scope here and we should stick to the bare minimum, which is proxy the GitHub calls to the server. Let's do just that here and take the rest in a different PR. |
replaces the old GitHubIssue which ran registry jobs directly with one that hits the registry api instead. also added integration tests that ensure various jobs can be kicked off as github issue events and we get the resulting comments, issue close events, etc.
|
9a8d1ba implements the "thin client" approach described in #649. The core idea is that
This tears out most of the heavy dependencies from the GitHub workflow — no more I added a couple of integration tests for GitHubIssue specifically which write the event to disk, then triggers the pipeline. The point of these is to test the operation completes, sure, but more specifically that we see the issue comments, issue close events, and trustee signing we expect. The file is kind of verbose with the helpers, but those helpers will come in handy later I hope for other tests! Along the way I fixed a few other things I noticed:
|
|
I put 6c023cf in a separate commit because it's a bigger structural change — we no longer have any reason for a But we do still need a way to indicate a log is more important than |
|
I'm also interested in adding a few more package fixtures so that we can e2e test the job queue. For instance, send 5 jobs at the same time, or send jobs of varying types and make sure the result order is what we expect (a job was picked up in priority order, matrix jobs finished last, etc.), that the compiler matrix completes, and so on. |
WIP, should fix #696, fix #649