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

Enable curl error retries. #5273

Merged
merged 4 commits into from
Sep 4, 2024
Merged

Conversation

ypatia
Copy link
Member

@ypatia ypatia commented Sep 3, 2024

Core-side retries on curl errors have been explicitly disallowed in the past in #3712 to protect from double writes in case of errors in a connection teardown phase, so after a fragment has been written, where a retry would just re-write the same fragment.

However, we have experienced spurious "Connection reset by peer" type of errors especially during ingestion (Write queries), which we think could be mitigating by allowing retries for most of curl errors. (exclude those that typically happen during connection teardown).

[sc-54377]


TYPE: IMPROVEMENT
DESC: Enable curl error retries.

@ypatia ypatia marked this pull request as ready for review September 3, 2024 14:04
@ypatia ypatia requested a review from a team as a code owner September 3, 2024 14:04
@ypatia
Copy link
Member Author

ypatia commented Sep 3, 2024

/backport to release-2.26

Copy link
Contributor

github-actions bot commented Sep 3, 2024

Started backporting to release-2.26: https://github.com/TileDB-Inc/TileDB/actions/runs/10684663732

Copy link
Contributor

@nickvigilante nickvigilante left a comment

Choose a reason for hiding this comment

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

Approved tiledb/api/c_api/config/config_api_external.h and tiledb/sm/cpp_api/config.h only.

@ypatia ypatia force-pushed the yt/sc-54277/enable_curl_error_retries branch from 42a2335 to 25393b8 Compare September 3, 2024 14:34
Copy link
Contributor

@shaunrd0 shaunrd0 left a comment

Choose a reason for hiding this comment

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

👍

tiledb/sm/rest/curl.cc Outdated Show resolved Hide resolved
@KiterLuc KiterLuc changed the title Enable curl error retries Enable curl error retries. Sep 3, 2024
@ypatia ypatia force-pushed the yt/sc-54277/enable_curl_error_retries branch from 78ed593 to d43b024 Compare September 4, 2024 07:17
Co-authored-by: Shaun M Reed <shaunrd0@gmail.com>
@ypatia ypatia force-pushed the yt/sc-54277/enable_curl_error_retries branch from d43b024 to 75a07f5 Compare September 4, 2024 07:22
@KiterLuc KiterLuc merged commit 0ac064b into dev Sep 4, 2024
62 checks passed
@KiterLuc KiterLuc deleted the yt/sc-54277/enable_curl_error_retries branch September 4, 2024 08:49
KiterLuc pushed a commit that referenced this pull request Sep 4, 2024
Backport of #5273 to release-2.26

---
TYPE: IMPROVEMENT
DESC: Enable curl error retries.

---------

Co-authored-by: Ypatia Tsavliri <ypatia@tiledb.com>
Co-authored-by: Shaun M Reed <shaunrd0@gmail.com>
@teo-tsirpanis
Copy link
Member

I think we should backpot this to 2.27 as well.

@KiterLuc
Copy link
Contributor

KiterLuc commented Sep 4, 2024

@teo-tsirpanis we might have to but we might get rid of the current release-2.27 branch so let's not do it now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants