search

cloudyr / googleCloudStorageR

Google Cloud Storage API to R
https://code.markedmondson.me/googleCloudStorageR
Other
104 stars 29 forks source link

Added retry with bucketLevel acl for resumeable uploads #164

Closed stuvet closed 2 years ago

stuvet commented 2 years ago

Submitted in response to https://github.com/cloudyr/googleCloudStorageR/issues/122#issuecomment-1192741059

Sorry about so many diffs - this was caused by running devtools::document(). I only changed lines 543 & 614-615 of uploads.R & included the httr::http_error function. Feel free to decline this one & fix it in a neater/different way. I'm not quite satisfied this approach.

Problems

  1. Resumable uploads (only) failing when predefinedAcl='private' is incorrectly set for a bucket which has uniform ACL.
  2. Resumeable uploads not retrying, unlike for simple uploads.

According to the api reference, when a predefinedAcl is supplied to a bucket with uniform permissions a 400 status code is returned.

In gcs_retry_upload, when predefinedAcl='private' is incorrectly supplied for a bucketLevel bucket:

Current Behaviour

  1. upload_status <- PUTme call returns a 400 status_code with an error message starting "Cannot insert legacy ACL..."
  2. Triggers the else clause
  3. Returns a stop("Couldn't get upload status").
  4. Propagates back to the error clause of the tryCatch in gcs_upload
  5. Mistakenly caught by the if(!grepl("Cannot insert legacy ACL", err$message)) {... condition, which prevents any retry, throwing the error instead.

Other potential issues:

Proposed Behaviour

  1. upload_status <- PUTme call returns a 400 status_code with an error message starting "Cannot insert legacy ACL...
  2. Triggers the else if (httr::http_error(upload_status)) {... clause
  3. Throws the original httr error message
  4. Propagates back to the error clause of the tryCatch in gcs_upload
  5. Correctly fails to be caught by the if(!grepl("Cannot insert legacy ACL", err$message)) {... condition, so resumable uploads are reattempted with predefinedAcl='bucketLevel', to match the simple upload behaviour.

Concerns

MarkEdmondson1234 commented 2 years ago

The approach looks good, to your questions

  • I am not entirely happy that all httr errors would now be propagated back to be retried - seems unnecessary, and you could just limit it to 400. - would prefer it to be only 400 status codes.

There are custom error classes for gar_api_generator() functions that would return a try exception http_400 which is missed using PUTme(), I may look at adding that. The relevant code in googleAuthR is https://github.com/MarkEdmondson1234/googleAuthR/blob/297e17847a07a55c80e80d8382342c8f383afdf9/R/utility.R#L2-7

abort_http <- function(status_code, msg = NULL){
  myMessage("Custom error", status_code, msg, level = 2)
  rlang::abort(paste0("http_",status_code), 
               message = paste0("http_", status_code, " ", msg)
  )
}

and then used via 

tryCatch(blah(),  http_400 = function(err) message("A 400 error! do something"))
  • Are you happy to expose the httr errors to the end user if they called gcs_retry_upload directly?

yes

  • I have not yet had a chance to test this with targets, so it may not fix my original problem!

please do try it to make sure, ideal would be a test that fails now but is fixed by your new code.

stuvet commented 2 years ago

I'm really sorry I missed this.

I used this reprex to validate this PR.

I'll put together a more targets focussed test tomorrow, but I've been using this PR for the last 7 days while developing a ~1400 target pipeline with ~600 of the targets creating files >5MB (many multi-GB).

The same pipeline previously triggered that error every time a certain collection of targets ran but hasn't yet been repeated since this change.

MarkEdmondson1234 commented 2 years ago

Thanks, is this then confirmed to fix #122 ?