Repository navigation
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces stall control capabilities for resumable uploads by adding configuration options for minimum transfer rate and stall timeout, and integrating a stall controller to monitor progress and handle recovery. The feedback highlights a critical NullPointerException in ResumableUploadChunkCoordinator due to accessing the uninitialized callContext during construction; it is recommended to initialize recoverySettings lazily within the recover method. Additionally, the merging logic in ResumableUploadOptions should be revised to prevent a custom global timeout from being silently cleared when merging with an empty options instance.
| this.progressTracker = checkNotNull(progressTracker, "progressTracker must not be null"); | ||
| this.stallController = stallController; | ||
| this.recoverySettings = createFirstRecoveryAttempt(); | ||
| this.buffer = new RewindableStreamBuffer(payload, chunkSize); |
There was a problem hiding this comment.
Remove the initialization of recoverySettings from the constructor because callContext is not yet initialized at construction time (it is null). Calling createFirstRecoveryAttempt() here will throw a NullPointerException when stallController is not null.
| this.progressTracker = checkNotNull(progressTracker, "progressTracker must not be null"); | |
| this.stallController = stallController; | |
| this.recoverySettings = createFirstRecoveryAttempt(); | |
| this.buffer = new RewindableStreamBuffer(payload, chunkSize); | |
| this.progressTracker = checkNotNull(progressTracker, "progressTracker must not be null"); | |
| this.stallController = stallController; | |
| this.buffer = new RewindableStreamBuffer(payload, chunkSize); |
| private TimedAttemptSettings createFirstRecoveryAttempt() { | ||
| if (stallController != null) { | ||
| return recoveryAlgorithm.createFirstAttempt( | ||
| callContext.withRetrySettings( | ||
| ResumableUploadCallableImpl.RETRY_SETTINGS.toBuilder() | ||
| .setTotalTimeoutDuration(Duration.ZERO) | ||
| .build())); | ||
| } | ||
| return recoveryAlgorithm.createFirstAttempt(); | ||
| } |
There was a problem hiding this comment.
Update createFirstRecoveryAttempt to accept ApiCallContext callContext as a parameter. This allows us to pass the non-null callContext when initializing recoverySettings lazily during the recovery process.
| private TimedAttemptSettings createFirstRecoveryAttempt() { | |
| if (stallController != null) { | |
| return recoveryAlgorithm.createFirstAttempt( | |
| callContext.withRetrySettings( | |
| ResumableUploadCallableImpl.RETRY_SETTINGS.toBuilder() | |
| .setTotalTimeoutDuration(Duration.ZERO) | |
| .build())); | |
| } | |
| return recoveryAlgorithm.createFirstAttempt(); | |
| } | |
| private TimedAttemptSettings createFirstRecoveryAttempt(ApiCallContext callContext) { | |
| if (stallController != null) { | |
| return recoveryAlgorithm.createFirstAttempt( | |
| callContext.withRetrySettings( | |
| ResumableUploadCallableImpl.RETRY_SETTINGS.toBuilder() | |
| .setTotalTimeoutDuration(Duration.ZERO) | |
| .build())); | |
| } | |
| return recoveryAlgorithm.createFirstAttempt(); | |
| } |
| if (madeProgressSinceRecovery) { | ||
| recoverySettings = | ||
| recoveryAlgorithm.createNextAttempt(recoveryAlgorithm.createFirstAttempt()); | ||
| recoverySettings = recoveryAlgorithm.createNextAttempt(createFirstRecoveryAttempt()); | ||
| } else { | ||
| recoverySettings = recoveryAlgorithm.createNextAttempt(recoverySettings); | ||
| } |
There was a problem hiding this comment.
Initialize recoverySettings lazily inside the recover method where callContext is guaranteed to be non-null. This avoids the NullPointerException during construction.
if (recoverySettings == null) {
recoverySettings = createFirstRecoveryAttempt(callContext);
}
if (madeProgressSinceRecovery) {
recoverySettings = recoveryAlgorithm.createNextAttempt(createFirstRecoveryAttempt(callContext));
} else {
recoverySettings = recoveryAlgorithm.createNextAttempt(recoverySettings);
}| } else if (builder.getTransferStallMinimumRate() != null | ||
| && builder.getTransferStallTimeout() != null) { | ||
| builder.setGlobalTimeout(null); | ||
| } |
There was a problem hiding this comment.
This logic causes a side-effect where merging an options instance that has both stall control and a custom globalTimeout with an empty/default ResumableUploadOptions instance will silently clear the custom globalTimeout. Merging with an empty/default instance should generally be a no-op. Consider if this behavior is truly desired, or if the disabling of the default global timeout should be handled solely in ResumableUploadFutureImpl.setUpGlobalTimeout().
References
- For resumable uploads, prevent infinite loops of recovery attempts by enforcing a global timeout rather than tracking and limiting consecutive recovery attempts.
|
|
WIP - not ready for review