fix #4408: allowing for users to set the exception handling behavior #4488

shawkins · 2022-10-10T22:10:01Z

Description

I couldn't come up with a good way to deal with this all under the covers, so it seemed best to introduce an explicit handler with a signature that's sufficient to cover our default behavior. This makes the final solution much closer to some of the earlier proposals. For the method:

boolean retryAfterException(boolean isStarted, Throwable t);

our current behavior is:

(b, t) -> b && !(t instanceof WatcherException)

That is we'll retry once started and only if it's not a WatcherException.

The expectation here is that if you use the runnableInformer method, you can change this behavior by calling something like exceptionHandler((b, t) -> true)

In reviewing the go implementation, I see the following differences:

Their WatchErrorHandler is doc'd as only informational - however the reflector is passed in, so you can easily kill it if you wanted.
They will always retry, so the WatchErrorHandler method does not return anything.
The equivalent of a watch onClose exception is not passed to their WatchErrorHandler. I think their assumption is that you'll immediately be trying a fresh list / watch and then you'll experience a problem with one of those calls if something is still wrong.

I've added a helper method to ExceptionHandler to check for deserialization issues - but that is not currently used in any built-in way. If you use something like exceptionHandler((b, t) -> true), but there is a jsonprocessing exception with the list call or with a watch event the result would be that you'll just keep retrying - a fresh list operation, then watch. If you don't want to make an assumption that things will self heal (a missing conversion webhook is installed later), then ExceptionHandler.isDeserializationException could be used - exceptionHandler((b, t) -> !ExceptionHandler.isDeserializationException(t))

With a major release we can then determine if the default behavior should become more generally to just keep retrying.

Along with this is a small breaking change to make the return type for start and stopped to be a CompletionStage rather than a CompletableFuture - I realized that since we don't want the user completing and don't support canceling, we should be using the narrower type instead. Since the stopped method was just introduced we may be able to change that one without too much concern (that would also need backported). If anyone has any qualms with start changing as well, I'll revert that.

Type of change

Bug fix (non-breaking change which fixes an issue)
Feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change
Chore (non-breaking change which doesn't affect codebase;
test, version modification, documentation, etc.)

Checklist

Code contributed by me aligns with current project license: Apache 2.0
I Added CHANGELOG entry regarding this change
I have implemented unit tests to cover my changes
I have added/updated the javadocs and other documentation accordingly
No new bugs, code smells, etc. in SonarCloud report
I tested my code in Kubernetes
I tested my code in OpenShift

…behavior

manusa · 2022-10-19T10:03:20Z

...ent/src/main/java/io/fabric8/kubernetes/client/informers/impl/SharedInformerFactoryImpl.java

@@ -106,11 +107,11 @@ public synchronized <T> SharedIndexInformer<T> getExistingSharedIndexInformer(Cl

  @Override
  public synchronized Future<Void> startAllRegisteredInformers() {
-    List<CompletableFuture<Void>> startInformerTasks = new ArrayList<>();
+    List<CompletionStage<Void>> startInformerTasks = new ArrayList<>();


This doesn't seem to be compliant with line 128.
I'm going to add a subsequent commit to ensure this won't cause any issue (there shouldn't be any issue because the underlying impl will be CompletableFuture, but the code is wrong nonetheless).