Http retry

[arnevogt]

add (configurable) retries for WPS Process

[nbrinckm]

I'm not 100% through the code here (I'm going to do that tomorrow), but the question: You did't add retries for establishing the connection to the wps server, did you? I found that I needed it in #60. I think it would make sense to adopt it to the configurability as you did it here in your branch.

[nbrinckm]

One thing that came into my mind yesterday: Can we solve the retry thing with a decorator pattern?

So that we can write something like this:

val processOutput = doAndRetryIfNeeded<WPSClientException>(retryConfig) { retryCount ->
    LOGGER.info("bla with ${retryCount}")
    wpsOutput = wpsClient.execute()
}

I think a decorator as this would be reusable for many different things - but I'm not sure if the implementation would be trivial. I think it would need some kind of static function, so that we can keep the this context of the class. I haven't tried it yet. But maybe this is something where it really makes sense to have kotlin & its power regarding higher order functions.

[arnevogt]

@nbrinckm

• adopted RetryUtils decorator for getCapabilites, getProcessDescription and executeProcess
• change retry semantics (retries do not include first attempt)

[nbrinckm]

Maybe we can add the retry also in the fetchContent method in the AbstractWrapper file.

Here we fetch the contents from the object storage - and http retry settings could be helpful as well. When I take a look at https://docs.oracle.com/en/java/javase/11/docs/api/java.net.http/java/net/http/HttpClient.html then it seems that this could throw IOExceptions or InterruptedException. I guess the IOException is the better one to catch here.

Same is true for the file storage upload call (currently line 435 in AbstractWrapper.kt). Here it makes sense to retry the upload if it fails due to network reasons. The exceptions here are quite a lot, however I added the wrote the upload method on my own in Java (due to a conflict with the object method name in the MinIOClient & the keyword in kotlin). We could just write our own Exception class that we can fill with any of the problematic exceptions from the MinIOClient & just catch our own exception in the retry.

We could also change the decorator to give it a predicate function that tests the exception. That way we could handle multiple exception types.

Could look like this:

val wpsProcess = retry<WPSProcess>(
  forExceptionsOfTypes(
     IOException::class.java., InterruptedException::class.java
  ), { it ->
     return@retry WPSProcess()
})

I think in that case we could even remove the exception type parameter from that function & just catch every possible exception - that we then put into the predicate.

But maybe all of those ideas go out of scope of that MR here, so feel free to make a separate issue if you think we can handle that better externally.

[nbrinckm]

My base points are the secondary constructor & maybe the improvements of the very last sleep call. Then I will approve the pull request. Rest is optional & can be made in other issues & pull requests.

[MichaelLangbein]

But maybe all of those ideas go out of scope of that MR here, so feel free to make a separate issue if you think we can handle that better externally.

Hi Nils! As for my part, I think indeed that the improvements you mentioned are better dealt with in a separate issue - just because I don't want you to have to keep working on this after the sprint is officially over. I've created one here: https://github.com/riesgos/async/issues/68 And here: https://github.com/riesgos/async/issues/69

[arnevogt]

@nbrinckm

pls re-review

• removed unnecessary sleep after final retry failed
• removed secondary constructor for WPSProcess
  • actually use retry config

[arnevogt]

I will address the other improvements in another PR.