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

ROX-17712: Remove updater onConflict retry #34

Closed
wants to merge 3 commits into from

Conversation

ludydoo
Copy link
Contributor

@ludydoo ludydoo commented Jun 22, 2023

The updater has some logic to retry an update in case of a conflict. But AFAIK, this retry would never lead to a successful response whenever there is a conflict in the first place.

This retry logic doesn't make sense because

  • A resource that was changed on the server will never be able to be updated before being refreshed
  • It will make the next attempts much more likely to fail, as it delays those attempts with a backoff

This PR removes the retry logic on updates

Copy link
Contributor

@porridge porridge left a comment

Choose a reason for hiding this comment

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

This looks good to me but we should definitely discuss this with @joelanford and forward this upstream if this helps in our case.

@SimonBaeumer
Copy link
Member

@ludydoo Can you open this PR upstream? If we get it merged upstream we can implement it downstream as well.
My main concern is that we diverge from upstream too much when we did not 100% understood the reasoning behind a removal.
If the removal of the retry fixes a bug which affects us we can remove it here as well directly.

@ludydoo
Copy link
Contributor Author

ludydoo commented Jun 27, 2023

for reference operator-framework/helm-operator-plugins#217

@ludydoo ludydoo force-pushed the ROX-17712-remove-onconflict-retry branch from dba6985 to b8fcfa6 Compare June 29, 2023 14:04
@ludydoo ludydoo closed this Jun 30, 2023
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.

None yet

4 participants