31 should retry on 5xx #62
Conversation
Codecov Report
@@ Coverage Diff @@
## master #62 +/- ##
============================================
+ Coverage 83.35% 83.63% +0.27%
- Complexity 259 262 +3
============================================
Files 36 36
Lines 721 727 +6
Branches 25 25
============================================
+ Hits 601 608 +7
+ Misses 119 118 -1
Partials 1 1
Continue to review full report at Codecov.
|
ghost
requested review from
Conflicts: src/main/java/com/commercetools/sync/services/impl/CategoryServiceImpl.java src/main/java/com/commercetools/sync/services/impl/InventoryServiceImpl.java
| @@ -53,8 +54,8 @@ | |||
| */ | |||
| public CategorySync(@Nonnull final CategorySyncOptions syncOptions) { | |||
| this(syncOptions, | |||
| new TypeServiceImpl(syncOptions.getCtpClient()), | |||
| new CategoryServiceImpl(syncOptions.getCtpClient())); | |||
| new TypeServiceImpl(getRetryOn5xxClient(syncOptions.getCtpClient())), | |||
There was a problem hiding this comment.
I wouldn't do it here on every time we need to get the client, but rather one time when it's injected and bound to the options.
There was a problem hiding this comment.
Ah, I forgot about the options. Good point 👍
| public class ClientConfigurationUtils { | ||
| private static BlockingSphereClient ctpClient; | ||
| private static final long DEFAULT_TIMEOUT = 30; | ||
| private static final TimeUnit DEFAULT_TIMEOUT_TIME_UNIT = TimeUnit.SECONDS; | ||
| private static final int RETRY_ON_5XX_ATTEMPTS_LIMIT = 27; |
There was a problem hiding this comment.
Also why is it a global variable and not passed as a parameter to the method it's used in?
There was a problem hiding this comment.
27 fits into proposed default strategy. More about it in a comment below.
|
|
||
| /** | ||
| * Given a {@link RetryContext} it returns duration for a current attempt. The duration is calculated from a | ||
| * following equation: <pre>duration = 2 ^ (((retryAttemptNumber - 1) / 3) + 1) [seconds]</pre> |
There was a problem hiding this comment.
could you please explain the idea behind this strategy?
There was a problem hiding this comment.
About the strategy: it is only my proposition. In fact I have no clue what retry strategy is wanted here. I have expressed my concerns some time ago in a issue #31 (comment)
This particular strategy seemed reasonable to me: first it performs few attempts with a little duration and then the time is increased. There is 27 attempts so it sum to almost 2 hours of waiting (e.g. when the server is down).
Please share your opinion about what is expected here @heshamMassoud
There was a problem hiding this comment.
retry strategy: https://github.com/sphereio/sphere-node-utils/blob/master/src/coffee/helpers/repeater.coffee#L90-L96
Provide through IT as an example, not function of sync..
Conflicts: src/main/java/com/commercetools/sync/inventories/InventorySync.java
Conflicts: src/main/java/com/commercetools/sync/commons/utils/ClientConfigurationUtils.java
Summary
Provide retry on server errors.
Description
getRetryOn5xxClientfunction inClientConfigurationUtils, that wrapsSphereClientinto aRetrySphereClientDecorator.2s, 2s, 2s, 4s, 4s, 4s, 8s, ... , 512s, for each failed request.Relevant Issues
#31
Todo
Hints for Review
Please express opinion about retry strategy, and if it should be more customizable within a method parameters.