fix #3271: refining the wait logic #3274
Conversation
|
Can one of the admins verify this patch? |
|
Like most things this turned out to be bigger than I thought at first glance... It makes sense to me to switch the implementation to be based upon an informer instead - as that already has the necessary retry logic and it's simple to express. With this change it is unnecessary to specifically configure an exponential backoff for this operation - it should use the defaults for watches / informers. I did not remove those fields from all of the relevant contexts as that greatly expands the scope of these changes. This change also reuses the existing KubernetesClientTimeoutException instead of using an IllegalArgumentException for timed out wait calls - I'll note that as a breaking change. This also simplifies the logic in NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicableListImpl - it was somewhat idiosyncratic and should be using the fork join pool for the potentially long running waitUntilCondition tasks. At some point the resource handlers and underlying logic should exposes the underlying futures as that would prevent this needless usage of threads. This addresses having duplicate metadata.name fields in the watch url, but the real problem is #3275 |
shawkins
force-pushed
the
wait
branch
2 times, most recently
from
5716562
to
1b1a26f
Compare
|
As mentioned on the issue, the subsequent commit removes the declared interruptedexception to simplify internal code and for consistency with the rest of the api. @manusa if that doesn't seem reasonable I can pull that out. |
| executor.shutdown(); | ||
| } | ||
| public List<HasMetadata> waitUntilReady(final long amount, final TimeUnit timeUnit) { | ||
| return waitUntilCondition(resource -> Objects.nonNull(resource) && getReadiness().isReady(resource), amount, timeUnit); |
There was a problem hiding this comment.
The statement
getReadiness().isReady(resource)is not equivalent to
handlerOf(resource).waitUntilReady(...)However the former doesn't make much sense since the effective call of waitUntilRady should be that of the applicable *OperationsImpl (e.g. ServiceHandler->ServiceOperationsImpl), especially considering the ServiceTest. Are we sure the Endpoints are being queried after these changes?
the only time to retry is on http gone delete does not need special handling in the watcher
also fully removing the additional operation/context fields
|
SonarCloud Quality Gate failed. |
InterruptedException is no longer thrown on websocket timeout, rather a KubernetesClientException. Unfortunately there is no way to differentiate between a timeout and an error. * fabric8io/kubernetes-client#3274 * fabric8io/kubernetes-client#3197
Description
To address #3271 the only time to retry is on http gone - the watch framework will already handle all of the other retries.
delete does not need special handling in the watcher
Type of change
test, version modification, documentation, etc.)
Checklist