Mrc 5600 - status endpoint #8
Conversation
R/api.R
Outdated
| ##' state root :: root | ||
| ##' state queue :: queue | ||
| report_run_status <- function(root, queue, job_id) { | ||
| queue$get_status(job_id) | ||
| } |
There was a problem hiding this comment.
you dont use root here, can just remove it
R/queue.R
Outdated
| time_started = if (!is.na(times[2])) scalar(times[2]) else NULL, | ||
| time_complete = if (!is.na(times[3])) scalar(times[3]) else NULL, |
There was a problem hiding this comment.
i think jsonlite (package we use for serialising to json) converts NA to null so i think will be fine
| time_started = if (!is.na(times[2])) scalar(times[2]) else NULL, | |
| time_complete = if (!is.na(times[3])) scalar(times[3]) else NULL, | |
| time_started = scalar(times[2]), | |
| time_complete = scalar(times[3]), |
if you want to test how something will look in json form you can run jsonlite::toJSON on it
R/queue.R
Outdated
| time_queued = scalar(times[1]), | ||
| time_started = if (!is.na(times[2])) scalar(times[2]) else NULL, | ||
| time_complete = if (!is.na(times[3])) scalar(times[3]) else NULL, | ||
| packet_id = if (status == "COMPLETE") scalar(rrq::rrq_task_result(job_id, controller = self$controller)) else NULL, |
There was a problem hiding this comment.
i think we should also handle the error case here, a task status can also be "ERROR", i found all the statuses here: https://github.com/mrc-ide/rrq/blob/ba2784057e8ad83e304ac9ae44081110c6b3d19c/R/common.R
for now we can ignore the other statuses
R/queue.R
Outdated
| log_dir_name <- "runner-logs" | ||
| dir.create(log_dir_name, showWarnings = FALSE) | ||
| worker_config <- rrq::rrq_worker_config(heartbeat_period = 10, logdir = log_dir_name) |
There was a problem hiding this comment.
i dont think the logs should be at root level, this is the git repo, we've put all the worker stuff in .packit when the workers come up, i think .packit/logs is a suitable place because then we'll have .packit/workers for the worker roots and .packit/logs for the logs
but maybe lets wait for what rich thinks
There was a problem hiding this comment.
@richfitz so had another think about this, 2 potential options can be:
./.packit/logsas log dir- we can have the logs dir in
/worker-logsat root level in the docker container which could be a logs volume shared by the workers?
There was a problem hiding this comment.
I really don't mind, so long as packit itself can't read them directly - only over the api. You could make this another command line switch perhaps, though it really only needs setting before the first worker ever turns up, it doesn't need doing each time as it is persisted in redis.
I wonder if using .packit/ will be confusing as this is an orderly detail so instead do
/logs/worker
/logs/task
as your directory, and add VOLUME /logs to the dockerfile?
…/orderly.runner into mrc-5600-status-endpoint
.gitignore
Outdated
| @@ -15,3 +15,4 @@ docs | |||
| .valgrind_ignore | |||
| inst/doc | |||
| pkgdown | |||
| logs | |||
There was a problem hiding this comment.
this should not be needed
R/queue.R
Outdated
| log_dir_name <- "logs/worker" | ||
| dir.create(log_dir_name, showWarnings = FALSE) | ||
| worker_config <- rrq::rrq_worker_config(heartbeat_period = 10, logdir = log_dir_name) |
There was a problem hiding this comment.
this is what i was talking about with the tempfile thing, if the logdir is instead an optional arg of the initialize function (so logdir = NULL in the function arg) then in our tests we can create a temp folder for them using tempfile or tempdir and we can pass in that path into the queue initialize, also if we dont pass it in then the logging can be disabled as not all the tests need it
tempfile or tempdir are great because the folders also get cleaned up after the tests so we dont need to deal with any of that
| } | ||
| }, | ||
| "status": { | ||
| "type": "string" |
There was a problem hiding this comment.
Might be nice to specify the list of allowed values here. Or would that be too restrictive?
There was a problem hiding this comment.
good idea have done as part of next PR
This PR adds the status endpoint. THe endpoint returns the status of a run with the following info: status, queue_position, time_queued, time_started, time_complete, packet_id, logs..... all these will return null if not available