Please simplify the code to avoid the duplication. In this case it shouldn't be necessary to handle the case where writeRequests.size() <= MAX_DYNAMODB_BATCH_ITEMS as a special case: the general code should handle that well - just like it can handle the last batch.

Why do we use int here?

This code can be simplified by creating a variable that holds an index of the last element last = std::min(writeRequests.size(), i + MAX_DYNAMODB_BATCH_ITEMS).

You can simplify the code by avoiding reusing the variables and using auto

I'm not a big fan of using auto where the type can be declared explicitly. Also, reusing the same variables is a part of loop optimization here (we don't create new ones inside the loop and reuse existing allocated memory for it).

outcome isn't used outside of this loop so we can declare it here

Same as above. I'm sure we should not use auto, where the type can explicitly be used, and allocating and reusing this variable here, is an optimized way from my perspective.

Also, reusing the same variables is a part of loop optimization here

Have you measured the performance difference? I don't think this type of premature optimization is beneficial - if the variable is used only locally, it is easier to reason about it: you don't have to think about its initial value and about possible usages after the loop. Also, having a variable defined inside a loop might enable some optimization (I'm guessing now). Currently, we only reuse the name - new variables are constructed in every iteration.

So overall, we have a lot more important performance issues and saving a couple of machine instructions wouldn't be noticeable. On the other hand, from readability point of view, this is a noticeable difference - you are certain that the variable is used only where it is valid.

Have you checked the document I linked? There's a note there, highlighted in red and marked as important

If DynamoDB returns any unprocessed items, you should retry the batch operation on those items. However, we strongly recommend that you use an exponential backoff algorithm. If you retry the batch operation immediately, the underlying read or write requests can still fail due to throttling on the individual tables. If you delay the batch operation using exponential backoff, the individual requests in the batch are much more likely to succeed.

Are we sure that this strong recommendation doesn't apply to us?

This is a good catch. I've dug into this and seems that there is a chance to lose some items if we don't check for unprocessed items. I've added the check for it and exponential backoff delay along with jitter for a delay. Btw, Google Cloud documentation describes such an approach too.

This is a part of exponential backoff, which indeed should make the delay grow exponentially with number of retries, but it isn't implemented correctly. If you think about the units here, you can realize what's wrong. backoffStep is expressed in time units, e.g. in ms and we raise it to a power, which would result in ms for the first iteration, ms^2 for the second and so on. This doesn't make sense to express a delay in time squared, does that?

A correct formula should include exponentiation of unitless quantity, e.g. a constant factor of 2. Something like:
backoffStep * std::pow(2, delayRetry) + jitterMs

There are some examples and articles to use the current algorithm, but I don't mind changing it, especially AWS documentation in an example suggesting using this instead of the current.
It was changed!

As a side note, we need to check if the algorithm isn't included in the SDK - see D4436

Replied to that comment there.

Thanks for deep digging into the problem and helpful comments ;)

Good point - have you done any research on whether it is maybe available in a library that we already use?

As for the formula - I'm not sure I understand how the formula you provided is different from the one that's already here.

You missed the point of my comment. My intention was to keep all BatchWriteItem s in the loop. But I now see that it would probably not make an improvement in the code as we have to distinguish the initial request from the rest of the requests which are done for unprocessed items. So I think we're good here.

Good point - have you done any research on whether it is maybe available in a library that we already use?

The AWS C++ SDK has its internal RetryStrategy which implements a backoff delay algorithm, which is the same as @palys-swm suggested to use instead of the original ones, it's used inside the AWS Client itself, but for the BatchWrite AWS Documentation and SDK examples suggests us to use/implement this delay and loop by ourself.

By the way, this is a good idea to move this backoff delay algorithm to the shared lib methods (eg. GlobalTools) and use it for the delays between reconnects in AMQP/RabbitMQ client in the Tunnelbroker.

I've created a follow-up for this as ENG-1381.

I'm pretty sure you haven't tested this condition, because you're assigning here instead of comparing.

Oh, thanks a lot! That's a nit. I'll fix it.

And even if == was used, it would still result in an infinite loop - >= should be used instead.

== comaprision here is used because we are using std::min at line 86. That's why delayMS can not be greater than maxBackoffTime and that's why I've used == instead of >= because of the delayMS can not be greater and it does not make sense to use >= here.