Skip to content

Conversation

@NotTheEvilOne
Copy link
Contributor

What this PR does / why we need it:
This PR removes added "retrying" logic for gardenlinux.s3.S3Artifacts as the underlying boto3 implementation has an advanced and upstream supported feature already build-in.

@vivus-ignis
Copy link
Contributor

What this PR does / why we need it: This PR removes added "retrying" logic for gardenlinux.s3.S3Artifacts as the underlying boto3 implementation has an advanced and upstream supported feature already build-in.

I know about retry mechanism in boto. The idea here is to have a unifying interface for retries across the library, as we need this behavior not only for s3.

@NotTheEvilOne
Copy link
Contributor Author

NotTheEvilOne commented Oct 2, 2025

What this PR does / why we need it: This PR removes added "retrying" logic for gardenlinux.s3.S3Artifacts as the underlying boto3 implementation has an advanced and upstream supported feature already build-in.

I know about retry mechanism in boto. The idea here is to have a unifying interface for retries across the library, as we need this behavior not only for s3.

It would be better to feed unified timeout values than overwriting and actually having a high chance of multiplying the retry times applied to S3 connections.

@vivus-ignis
Copy link
Contributor

What this PR does / why we need it: This PR removes added "retrying" logic for gardenlinux.s3.S3Artifacts as the underlying boto3 implementation has an advanced and upstream supported feature already build-in.

I know about retry mechanism in boto. The idea here is to have a unifying interface for retries across the library, as we need this behavior not only for s3.

It would be better to feed unified timeout values that overwriting and actually having a high chance of multiplying the retry times applied to S3 connections.

Got you. How about we set boto retries to 0 to avoid the multiplying effect?

@NotTheEvilOne
Copy link
Contributor Author

Got you. How about we set boto retries to 0 to avoid the multiplying effect?

I would still prefer to use the boto3 way of handling retries as I see a high risk that we "break" upstream intended logic otherwise if we just add an overarching retry context manager to individual methods in our library and ignoring the effort that has been taken upstream.

@NotTheEvilOne NotTheEvilOne added the bug Something isn't working label Oct 2, 2025
@NotTheEvilOne NotTheEvilOne self-assigned this Oct 2, 2025
@vivus-ignis
Copy link
Contributor

Got you. How about we set boto retries to 0 to avoid the multiplying effect?

I would still prefer to use the boto3 way of handling retries as I see a high risk that we "break" upstream intended logic otherwise if we just add an overarching retry context manager to individual methods in our library and ignoring the effort that has been taken upstream.

Alright. I'm fine with that, but please don't remove the library. I see it can be useful in other places in this library.

@NotTheEvilOne NotTheEvilOne force-pushed the fix/remove-duplicated-retry-logic branch from 0577fa0 to c0cabdb Compare October 2, 2025 07:59
@NotTheEvilOne
Copy link
Contributor Author

Alright. I'm fine with that, but please don't remove the library. I see it can be useful in other places in this library.

I agree but we are not using it at the moment. As we use Git we can always return to the state where this library has been added once the functionality is needed.

@NotTheEvilOne NotTheEvilOne force-pushed the fix/remove-duplicated-retry-logic branch from c0cabdb to 1efe214 Compare October 2, 2025 08:02
@codecov
Copy link

codecov bot commented Oct 2, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.62%. Comparing base (6ba28d5) to head (1efe214).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #209      +/-   ##
==========================================
- Coverage   87.66%   87.62%   -0.05%     
==========================================
  Files          33       33              
  Lines        1549     1543       -6     
==========================================
- Hits         1358     1352       -6     
  Misses        191      191              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@vivus-ignis vivus-ignis left a comment

Choose a reason for hiding this comment

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

I'm ok with the change in general, but I don't get all these formatting changes to be honest. What tool do you use for formatting?

@NotTheEvilOne NotTheEvilOne merged commit 1199aab into main Oct 3, 2025
9 of 11 checks passed
@NotTheEvilOne NotTheEvilOne deleted the fix/remove-duplicated-retry-logic branch October 3, 2025 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants