Repository navigation
feat(ingest): accept uploads and queue them as jobs - #437
Conversation
POST/GET /ingest return 202 with a job id instead of holding the
request open while every archive member is processed, which caused
nginx 504s on large zips. Poll GET /ingest/jobs/{job_id} for status.
One worker, started by docker-entrypoint.sh beside Gunicorn, runs
queued jobs from the new ingest_job table. Clients that read the old
200 Ingest200 body must poll the job instead.
renato2099
left a comment
renato2099
left a comment
There was a problem hiding this comment.
This is a very nice improvement @lewismc , thank you!
There are a few minor things that we should polish before shipping this, but overall I think it is good. There are two main points I would like to make:
- The one process for processing files might have be revisited in the future once we need more parallelism.
- This also doesn't really fix our core issue with data processing
But we should follow those up separately and those shouldn't be a blocker for this PR
|
keep |
POST /ingest/jobs streams a .txt, .zip, .tar.gz, or .tgz up to 2 GiB and returns 202. One worker drains the ingest_job queue. Synchronous /ingest still finishes inside the request and rejects bodies over 100 MiB. The eTUFF migration matches histogram rows without joining every time-series row, and resolves position times with one indexed lookup. submission.filename stores the eTUFF name. Finished jobs are purged by cron. fswatch and the upload page use the job API. Archive extraction uses the Python standard library.
|
Follow-up on the review summary: one worker is still the limit for this PR, and the migration cost of a large eTUFF file is a separate problem. Neither blocks this change. Inline replies are on the original threads. |
The job upload opened its staging file inside an async function, and repeated error strings were duplicated literals.
bash 5 treated a zero-second read as another copy of the previous path, so later drops never reached the job API. The watcher also exited on startup because its state volume was not writable.
The contract test read the pending list as soon as the second POST returned, before the watcher finished writing that job.
renato2099
left a comment
renato2099
left a comment
There was a problem hiding this comment.
thanks for the changes @lewismc ! there are still a few things that we will have to circle back to though, but we can do that in the next release
| ); | ||
| fileInput.addEventListener("change", updateFileListDisplay); | ||
|
|
||
| function loadJobs() { |
There was a problem hiding this comment.
cool! you made the improvements on this file as well
one thing to note is that we will have to split this file shortly, it is already quite large+complex (it's not going to be maintainable by humans 😉 at this rate)
There was a problem hiding this comment.
Yes, I'm going to create a follow-on issue for this.
| FROM data_profile | ||
| WHERE position_date_time IS NULL | ||
| ) AS m | ||
| CROSS JOIN LATERAL ( |
There was a problem hiding this comment.
I think this makes sense, but ideally this should have been it's own separate PR such that we can
really validate that the results we are getting are consistent, I am also ok letting this part go in and
coming back to it later on
There was a problem hiding this comment.
Yes I'll create a new issue for this change. We should have a single SQL source now as well. I'll create the issue for that.
| send, | ||
| 413, | ||
| _TITLE_TOO_LARGE, | ||
| "Ingest jobs accept at most 2 GiB.", |
There was a problem hiding this comment.
this string should ideally use variable we defined, otherwise it is easy that they go out of sync
There was a problem hiding this comment.
Yes this was a placeholder. I'll remove the objectivity... however we need to figure out how to ingest files lager than 2GB as well...
| from tagbase_server.utils.io_utils import TEMP_DIR | ||
|
|
||
| SYNC_MAX_BYTES = 100 * 1024 * 1024 | ||
| JOB_MAX_BYTES = 2 * 1024 * 1024 * 1024 |
There was a problem hiding this comment.
if we want to enforce an upper limit, we should consider it post decompression no? 🤔 I guess it doesn't matter too much right now ... maybe something to think about for later as well
There was a problem hiding this comment.
Hmmm... if the file decompresses > available container disk space then we have a candidate "zip bomb". Lets keep this ceiling for now and inspect use cases. This may be big enough... who knows?
| - ingest | ||
| x-codegen-request-body-name: body | ||
| x-openapi-router-controller: tagbase_server.controllers.ingest_controller | ||
| /ingest/jobs: |
There was a problem hiding this comment.
we will have to change this name in the future as well 😅 as per classic REST API design we should use nouns for resources , so we would have something jobs with its own POST/GET/LIST and if we wanted to make it async, we could have a parameter for it ?sync=true or having /ingestions/sync + /ingestions/async, but we can also do that in a follow up
There was a problem hiding this comment.
Yes we are going back to my previous comments. I agree with you. You and I know there are a lot of implicit implications with this design decision. I am +1. Lets do it later.
|
|
Follow-ups from the review, for the next release:
#446, #447, and #448 already cover the earlier review notes. |
Merge origin/main, including queued-upload ingest from #437. Keep the deferred time-series date index rebuild in both schema files.
Merge origin/main, including queued-upload ingest from #437. Keep the deferred time-series date index rebuild in both schema files.
2 participants
POST/GET /ingest return 202 with a job id instead of holding the request open while every archive member is processed, which caused nginx 504s on large zips. Poll GET /ingest/jobs/{job_id} for status. One worker, started by docker-entrypoint.sh beside Gunicorn, runs queued jobs from the new ingest_job table. Clients that read the old 200 Ingest200 body must poll the job instead.