commercetools / commercetools-sdk-java-v2

The e-commerce SDK from commercetools for Java.
https://commercetools.github.io/commercetools-sdk-java-v2/javadoc/index.html
Apache License 2.0
34 stars 15 forks source link

Retry with new Version Handling #276

Closed bgrambichlerpixelart closed 2 years ago

bgrambichlerpixelart commented 2 years ago

I need to implement a retry handling with an updated version if i get a "ConcurrentModification" - Exception.

In SDK v1 we did that with RetrySphereClientDecorate and RetryRule classes.

public static SphereClient ofRetry(final SphereClient delegate) {
        final int maxAttempts = 5;
        final List<RetryRule> retryRules = Arrays.asList(RetryRule.of(
            RetryPredicate.ofMatchingStatusCodes(BAD_GATEWAY_502, SERVICE_UNAVAILABLE_503, GATEWAY_TIMEOUT_504),
            RetryAction.ofScheduledRetry(maxAttempts, context -> Duration.ofSeconds(context.getAttempt() * 2))),
            RetryRule.of(isDeleteAndNewVersionIsKnown(), retryWithNewVersion()),
            RetryRule.of(isUpdateAndNewVersionIsKnown(), retryWithNewVersion())
        );
        return RetrySphereClientDecorator.of(delegate, retryRules);
    }
And than manipulated the HttpRequestBody to insert the current version of the Exception Message

private static HttpRequestIntent getRequestIntentForMethod(HttpRequestIntent original, Long currentVersion) {
        if (original.getHttpMethod() == HttpMethod.POST) {
            final String body = ((StringHttpRequestBody) original.getBody()).getString()
                .replaceAll("\"version\":\\d+", "\"version\":" + currentVersion);
            return HttpRequestIntent.of(original.getHttpMethod(), original.getPath(), original.getHeaders(), StringHttpRequestBody.of(body));
        } else {
            final String path = original.getPath().replaceAll("\\bversion=\\d+", "version=" + currentVersion);
            return original.withPath(path);
        }
    }

In SDK v2 i tried to implement this usecase with RetryMiddleware class but i found no way to delegate the ProjectApiRoot Client to set a new Request with the manipulated HttpRequest.

One way could be, to surround every API-Request where we faceing the ConcurrentModification Problem with a while-Loop and try-catch handling to send API-request several times with updated order-version?!?

jenschude commented 2 years ago

Would this help? https://github.com/commercetools/commercetools-sdk-java-v2/blob/main/commercetools/commercetools-sdk-java-api/src/integrationTest/java/commercetools/ConcurrentModificationTest.java

It has been introduced in the 8.0.0

bgrambichlerpixelart commented 2 years ago

Cool, thats excactly what i need! I worked with Release 8.0.0-beta.3 and i didn't see that before. The piece of code is new, isn't it? Thanks a lot

jenschude commented 2 years ago

Yes it's new and shiny.

Also still not yet completely happy with it and still thinking about a possibility to get it working in a middleware. But as a matter of fact atm middlewares are running on the HTTP abstraction layer and don't know about the API abstraction layer which makes it harder to change the request itself.

jenschude commented 2 years ago

I added now a new Middleware which is able to update the body with the new version number and uses failsafe for backoff and multiple retries.

jenschude commented 2 years ago

Just released the 8.1.0 which adds the ConcurrentModificationMiddleware.

Please see https://github.com/commercetools/commercetools-sdk-java-v2/blob/9a96df6be87be0a177714d8ef7227b237598273b/commercetools/commercetools-sdk-java-api/src/integrationTest/java/commercetools/ConcurrentModificationTest.java#L49-L81

milindsingh commented 2 years ago

@jenschude Our ERROR logs are getting filled with 409 errors i.e. ERROR POST https://api.australia-southeast1.gcp.commercetools.com/xyz-abc/payments/fbbb8a22-2f19-4108-aa0e-b09b25cc98a4 409.

Since, we are handling this with a retry, can you suggest any way to make it a INFO type log ?

jenschude commented 2 years ago

I would configure this specific logger to only log higher error levels

jenschude commented 2 years ago

Ah this may be not working as it comes from the LoggerMiddleware which by default logs all ApiHttpExceptions as errors.

I may introduce a new configuration for this as like as the response and deprecation log event to configure the level for specific exceptions

jenschude commented 2 years ago

Please see 5f6df55

I added a configuration option for the InternalLogger to be able to globally configure the log level for exceptions and also on a per class level. Will be available with the next release

milindsingh commented 2 years ago

Thanks @jenschude

I would very helpful if you can add a example to configure InternalLogger to disable 409 logs.

jenschude commented 2 years ago

3106c8c

I downgraded the log from error to info for ConcurrentModificationExceptions by default and also fixed an issue with the exception level map

milindsingh commented 2 years ago

Hi @jenschude I am still seeing 409 ERROR logs in SDK 8.7 version. Do I need to change anything ?

Thanks

jenschude commented 2 years ago

By default ConcurrentModification is LogLevel INFO. I added a small test to show how to configure the specific loglevels with the ApiRootBuilder

https://github.com/commercetools/commercetools-sdk-java-v2/blob/1990c415a7771d006a944b7dbd1fff22932ad9fb/commercetools/commercetools-sdk-java-api/src/integrationTest/java/commercetools/ExceptionTest.java#L82-L89

jenschude commented 2 years ago

I found the culprit. When the import java.util.* was added to the InternalLoggerMiddleware the exceptionLevelMap was initialized with java.util.ConcurrentModificationException instead of the SDK one. Will add an additional test for it and release a patch version.

jenschude commented 2 years ago

Just released the version 8.7.1 which fixes the issue