Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(gensupport): per-chunk transfer timeout configs #2865

Merged
merged 16 commits into from
Nov 20, 2024

Conversation

Tulsishah
Copy link
Contributor

@Tulsishah Tulsishah commented Nov 8, 2024

feat(gensupport): per-chunk transfer timeout configs

Allow users to configure the per-chunk transfer timeout for retries
that's used during resumable uploads.

Needs to be exposed via the manual layer for storage.

Tested the feature(with default timeout and with some random value) with storagetestbench server emulator.

@Tulsishah Tulsishah requested a review from a team as a code owner November 8, 2024 04:14
@codyoss codyoss requested a review from tritone November 8, 2024 13:59
internal/gensupport/retry.go Outdated Show resolved Hide resolved
internal/gensupport/resumable_test.go Outdated Show resolved Hide resolved
internal/gensupport/resumable.go Outdated Show resolved Hide resolved
@Tulsishah Tulsishah force-pushed the transfertimeout_init branch from f81a9ee to f54010a Compare November 12, 2024 06:29
@Tulsishah Tulsishah requested a review from tritone November 12, 2024 06:42
@Tulsishah Tulsishah force-pushed the transfertimeout_init branch from f54010a to 007f67a Compare November 12, 2024 15:29
Copy link
Contributor

@tritone tritone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few more points here.

googleapi/googleapi.go Outdated Show resolved Hide resolved
googleapi/googleapi.go Outdated Show resolved Hide resolved
internal/gensupport/resumable.go Outdated Show resolved Hide resolved
internal/gensupport/retry.go Outdated Show resolved Hide resolved
internal/gensupport/resumable.go Outdated Show resolved Hide resolved
@Tulsishah Tulsishah force-pushed the transfertimeout_init branch from 007f67a to c9d6842 Compare November 13, 2024 05:47
@Tulsishah Tulsishah requested a review from tritone November 13, 2024 05:57
Copy link
Contributor

@tritone tritone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few more comments here.

googleapi/googleapi.go Outdated Show resolved Hide resolved
internal/gensupport/resumable.go Show resolved Hide resolved
internal/gensupport/resumable.go Show resolved Hide resolved
internal/gensupport/resumable.go Outdated Show resolved Hide resolved
@Tulsishah Tulsishah force-pushed the transfertimeout_init branch from 40ba714 to 00ceb49 Compare November 19, 2024 15:45
@Tulsishah Tulsishah requested a review from tritone November 19, 2024 16:04
Copy link
Member

@codyoss codyoss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM to me overall, I will wait for @tritone to fully sign-off. I will say this function is starting to get a bit long and complex. I would consider breaking up the loop body into some different functions can be better documented. Then you can use things like defer again too.

@tritone
Copy link
Contributor

tritone commented Nov 20, 2024

LGTM to me overall, I will wait for @tritone to fully sign-off. I will say this function is starting to get a bit long and complex. I would consider breaking up the loop body into some different functions can be better documented. Then you can use things like defer again too.

It's a good point; I can do this in a subsequent PR.

@Tulsishah Tulsishah requested a review from codyoss November 20, 2024 04:44
@codyoss codyoss added the automerge Merge the pull request once unit tests and other checks pass. label Nov 20, 2024
@gcf-merge-on-green gcf-merge-on-green bot merged commit 09fa125 into googleapis:main Nov 20, 2024
5 checks passed
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants