Skip to content

Reset offsets in buffer list for retries#5220

Merged
ypatia merged 1 commit intodevfrom
yt/sc-49128/fix_curl_retries
Aug 2, 2024
Merged

Reset offsets in buffer list for retries#5220
ypatia merged 1 commit intodevfrom
yt/sc-49128/fix_curl_retries

Conversation

@ypatia
Copy link
Copy Markdown
Member

@ypatia ypatia commented Jul 30, 2024

There's a bug in the retry logic when a 503 is encountered while uploading data. Our curl client fails to reset its BufferList state back to zero in this case and we end up sending a bunch of garbage data to the cloud service.

This PR fixes the issue and adds a regression test that failed before and passes with the fix.

[sc-49128]


TYPE: BUG
DESC: Reset offsets in buffer list for retries

@ypatia ypatia force-pushed the yt/sc-49128/fix_curl_retries branch 2 times, most recently from 9cc3684 to b647566 Compare July 30, 2024 12:32
@ypatia ypatia requested a review from shaunrd0 July 30, 2024 12:32
@ypatia ypatia force-pushed the yt/sc-49128/fix_curl_retries branch from b647566 to f35be0b Compare July 30, 2024 12:34
@Shelnutt2
Copy link
Copy Markdown
Contributor

Is there any changes needed related to buffer_list_seek_callback or CURLOPT_SEEKFUNCTION? We use this for the redirection which I believe is working fine but since it involves a similar type of reply of the buffers wanted to ask.

@ypatia
Copy link
Copy Markdown
Member Author

ypatia commented Jul 31, 2024

Is there any changes needed related to buffer_list_seek_callback or CURLOPT_SEEKFUNCTION? We use this for the redirection which I believe is working fine but since it involves a similar type of reply of the buffers wanted to ask.

Thanks for bringing this to my attention, I think this change is generic and applies to any retry with data stored in a BufferList structure, so it'll work for seek same as it works for plain read memory cb.

@ypatia ypatia merged commit 2a30fbe into dev Aug 2, 2024
@ypatia ypatia deleted the yt/sc-49128/fix_curl_retries branch August 2, 2024 07:47
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