Wait to fetch image data a few times in Watchdog. #4656
Conversation
server/src/watchdog.js
Outdated
| return new Promise((resolve, reject) => { | ||
| const tryGetBytes = function() { | ||
| getRawBytesPromise(DELAY).then(obj => { | ||
| if (obj === null) { |
There was a problem hiding this comment.
The beauty of Promise code is that it's not necessary to manually maintain a counter.
What if getRawBytesPromise just did something like:
// Three attempts with 1000 msec delay
getRawBytesPromise(1000)
.then(bytes => { return bytes ? bytes : getRawBytesPromise(1000) })
.then(bytes => { return bytes ? bytes : getRawBytesPromise(1000) })
.then(bytes => { bytes ? resolve(bytes) : reject(new Error(...)) }
I think this approach would be simpler and more idiomatic.
There was a problem hiding this comment.
Er, sorry, I meant "what if getImageRawBytes did something like..."
There was a problem hiding this comment.
I'd argue in this case the counter is not necessary because we call the function three times. Promises don't have anything to do with it. 😃 It is easier to read though.
There was a problem hiding this comment.
Currently the code retries on the first two rejections from Shot.getRawBytesForClip and rejects with its Error on the third. I don't think we can switch to the proposed promise chain without losing the original Error from Shot.getRawBytesForClip. In order for getRawBytesPromise to return something falsey, it needs to ignore the rejections.
There was a problem hiding this comment.
Sure, you could handle the rejection case any number of ways while still getting rid of the manual iteration counting code. We can chat about this tomorrow after standup 👍
There was a problem hiding this comment.
Sorry, I wasn't clear: I want to reject if it's null but retry two times if it's a rejection. Shot.getRawBytesForClip returns null when a (not {deleted,blocked,expired}) image is not found in the DB, so that's not an AWS/S3 related issue.
server/src/watchdog.js
Outdated
| setTimeout(() => resolve(), waitInMs); | ||
| }); | ||
| }; | ||
| const getRawBytesPromise = function(timeToWait) { |
There was a problem hiding this comment.
Probably couldn't hurt to move getRawBytesPromise and the delay function out to top level, to avoid redeclaring them over and over.
There was a problem hiding this comment.
hmmm I did that to minimize the scope of the functions. I think it wouldn't hurt anything? I'll move them up.
chenba
dismissed
jaredhirsch’s stale review
Added more commits based on feedback.
|
Pushed additional commits for ease of review. For the promise chain approach, I added a flag for retrying on |
Fixes #4612