Use a different storr namespace for each status #380
Conversation
Codecov Report
@@ Coverage Diff @@
## master #380 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 65 65
Lines 5489 5508 +19
=====================================
+ Hits 5489 5508 +19
Continue to review full report at Codecov.
|
|
Before, getting/setting worker status meant reading/writing the contents of files. Now, getting status means checking if a file exists, and setting status means removing one file and creating another. I am not sure which is faster, and it probably depends on the user's file system. However, this design seems a lot safer when it comes to potential race conditions I may have missed. The code is also a little cleaner, and it has the potential to be faster. So even though speed appears just a hair worse for @krlmlr's example (we now have |
|
I so apparently, processes started hanging unpredictably in my tests. I think it has something to do with this PR, so I reverted it. The code is still in the |
| silent = TRUE | ||
| ) | ||
| ) | ||
| if (is.character(out)){ |
There was a problem hiding this comment.
The try() pattern seems to be unsafe here:
is.character(try(stop()))
#> [1] TRUE
class(try(stop()))
#> [1] "try-error"Created on 2018-05-06 by the reprex package (v0.2.0).
How about the following general function:
set.seed(123)
flaky <- function(p) {
print("Side effect")
if (p < runif(1)) stop("oops")
p
}
retry <- function(code) {
quo <- rlang::enquo(code)
repeat {
tryCatch(
return(rlang::eval_tidy(quo)),
error = identity
)
Sys.sleep(0.2)
}
}
retry(flaky(0.1))
#> [1] "Side effect"
#> [1] "Side effect"
#> [1] "Side effect"
#> [1] "Side effect"
#> [1] "Side effect"
#> [1] "Side effect"
#> [1] 0.1Created on 2018-05-06 by the reprex package (v0.2.0).
| mc_set_not_ready <- function(worker, config){ | ||
| mc_set_status(worker = worker, status = "not ready", config = config) | ||
| mc_unset_status <- function(worker, config){ | ||
| lapply( |
There was a problem hiding this comment.
Can we somehow make the old worker status available here, to avoid the iteration? This code could live on as a new mc_purge_status() function, perhaps useful for initialization.
Summary
Here, I implement @krlmlr's suggestion to create a
storrnamespace for each persistent worker (ref: #369 (comment)). There are no speed improvements yet, but this approach to the cache is much better.GitHub issues addressed
Checklist
drake's code of conduct, and I agree to follow its rules.testthatunit tests totests/testthatto confirm that any new features or functionality work correctly.devtools::check()