Conversation
| $totalTimeoutMillis = $resumableUploadOptions['totalTimeoutMillis'] | ||
| ?? ($stallControlEnabled ? null : self::DEFAULT_TOTAL_TIMEOUT_MILLIS); |
There was a problem hiding this comment.
Nit:
Not sure I love the coalescing operator AND a tertiary operator together. It feels a bit loaded.
Hectorhammett
left a comment
Hectorhammett
left a comment
There was a problem hiding this comment.
I have mostly nits. Approving them as I leave them to your discretion @bshaffer
| if ($state->isStallControlEnabled()) { | ||
| $chunkTimeout = $state->calculateNextChunkTimeout($chunkSizeMiB); | ||
| if ($globalDeadlineMs !== null) { | ||
| $globalRemaining = ($globalDeadlineMs / 1000.0) - $now; |
There was a problem hiding this comment.
This is an absolute minute nit, but I like the idea of moving this 1000.0 into a constant:
$globalRemaining = ($globalDeadlineMs / self::MILLS_IN_A_SECOND)I just like having consistent naming and text that actually reminds immediately what are we doing for numbers. Of course this is extremely minute, so not blocking this review on this of course.
| $this->buffer = null; | ||
| } | ||
|
|
||
| if ($this->buffer === '') { |
There was a problem hiding this comment.
nit:
if (empty($this->buffer)) {
$this->buffer = null;
}
I know they are equivalent, but I believe that using empty is a more clear convention of not wanting it to be empty.
| $options['credentials'] = new \Google\ApiCore\InsecureCredentialsWrapper(); | ||
| } else { | ||
| $options['hasEmulator'] = true; | ||
| $options['credentials'] = new \Google\ApiCore\InsecureCredentialsWrapper(); |
There was a problem hiding this comment.
Let's use the use clause instead of using the path to use the class.
2 participants
Adds stall control support to Resumable Uploads according to go/cloudsdk-scotty-stall-control.
Changes
transferStallMinimumRate(MiB/s) andtransferStallTimeout(seconds) options tostartUpload().min(chunk_timeout, 2 * chunk_size / rate)).ApiException("Upload stalled.") when global deadline is not set or not exceeded.