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

Update waitUntilCondition to use a watcher instead of polling. #2239

Merged
merged 5 commits into from
May 22, 2020

Conversation

bbeaudreault
Copy link
Contributor

There were a couple different implementations of waitUntil*, and this PR unifies all of them on an approach that somewhat mirrors the native go watcher code. I couldn't fully mirror that approach because of the typing we do in the WatchConnectionManager and elsewhere. I think this is a good incremental step towards that, and we can do any necessary refactoring in those manager classes in another PR down the line if necessary.

When you call waitUntilCondition or waitUntilReady, the code will first just try to fetch the item once from the server. If the item matches the condition it returns early. Otherwise it proceeds to create a watcher. Any errors in the watcher will cause a retry, except HTTP_GONE which will propagate. We will retry with backoff until the timeout is met.

I wanted to make the backoff configurable, so I added the plumbing necessary to support a withWaitRetryBackoff method. It defaults to 5ms initial backoff with 2x exponential backoff growth.

Fixes #2204

…itUntil* methods should end up calling waitUntilCondition. Retry all exceptions except HTTP_GONE, with backoff
@centos-ci
Copy link

Can one of the admins verify this patch?

@rohanKanojia
Copy link
Member

ok to test

@bbeaudreault
Copy link
Contributor Author

This is ready for review. SonarCloud failed again due to coverage, but I think there is reasonably good coverage for the code I changed. I added a bunch of unit tests for WaitForConditionWatcher, and a bunch of mock tests in kubernetes-test for the new functionality of waitUntilCondition.

@rohanKanojia
Copy link
Member

@geoand : Hi, Could you please review this PR whenever you find some time? I remember you contributed initial implementation for waitUntilCondition in #1227

Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

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

It's been a long time since I looked at this code, but the change seems reasonable to me

Copy link
Member

@manusa manusa left a comment

Choose a reason for hiding this comment

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

Besides the nitpicking comment, looks great to me. Thx for contributing!

@rohanKanojia
Copy link
Member

@bbeaudreault : There seems to some compilation error. Could you please address them whenever you get time?

@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 20 Code Smells

17.6% 17.6% Coverage
0.0% 0.0% Duplication

@bbeaudreault
Copy link
Contributor Author

Ok everything is working again. Something had merged into master which turned one of my function calls ambiguous.

@manusa
Copy link
Member

manusa commented May 22, 2020

[merge]

@bbeaudreault
Copy link
Contributor Author

@manusa looks like the merge build failed:

Failed to execute goal org.jacoco:jacoco-maven-plugin:0.8.4:report (default-report) on project kubernetes-karaf-itests: An error has occurred in JaCoCo report generation.: Error while creating report: null: EOFException -> [Help 1]

@rohanKanojia
Copy link
Member

[merge]

@manusa
Copy link
Member

manusa commented May 22, 2020

@manusa looks like the merge build failed:

Failed to execute goal org.jacoco:jacoco-maven-plugin:0.8.4:report (default-report) on project kubernetes-karaf-itests: An error has occurred in JaCoCo report generation.: Error while creating report: null: EOFException -> [Help 1]

It happens sometimes, we need to fix it.

Retry has been started ;)

@fusesource-ci fusesource-ci merged commit 15d3ce5 into fabric8io:master May 22, 2020
@bbeaudreault
Copy link
Contributor Author

Great, thanks!

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.

BaseOperation#waitUntilCondition never fetches state from server
7 participants