Clear the counter on successful reconnect.

We should not throw a fatal error here. In case of the connection is lost it will throw a fatal error instead of going forward to the reconnection loop. It was changed to a log error.

Another idea is to store std::chrono::milliseconds(3000)) directly as a constant - that is probably the best solution

Shouldn't we increase the counter before calling connectInternal? Currently, the following can happen:

1. connect is called
2. connectInternal is called and it works - reconnectAttempt is set to 0 in onReady
3. reconnectAttempt is increased
4. next loop iterations with unsuccessful connects

I think it doesn't matter here to swap these lines because of:
In case of a successful connection, we are resetting the counter inside the connectInternal() but we have a condition to check it in a while loop above/outside it. Also, if we swap them the log message below will have a 0 attempt after the first connection lost.

The starting value will be reconnectAttempt = 1, so effectively we will try to reconnect only AMQP_RECONNECT_MAX_ATTEMPTS - 1 times.

Seems you're right. We can fix it by changing the condition above from this->reconnectAttempt < AMQP_RECONNECT_MAX_ATTEMPTS to this->reconnectAttempt <= AMQP_RECONNECT_MAX_ATTEMPTS.

What do you think about having exponential backoff also here? It should be really easy to implement, because x * 2 ^ reconnectAttempt can be used as sleep duration.

There is already a follow-up task ENG-1381 to switch exponential backoff.

Having a unit as a part of name is a lot better than comments with explanation - they will go out of sync.

Agree! I've changed to use AMQP_RECONNECT_ATTEMPT_INTERVAL_MS.

Another idea is to store std::chrono::milliseconds(3000)) directly as a constant - that is probably the best solution

This constant will be removed by the exponential backoff algorithm in the future by ENG-1381 so we can omit to make now. Thanks for the suggestion!

where do these numbers come from?

The reasonable maximum wait timeout to reconnect is around 30 seconds, and the reasonable attempt wait to reconnect is 2-5 seconds speaking for the rabbitMQ. That's why I went to 10 attempts by 3 seconds.

Shouldn't we increase the counter before calling connectInternal? Currently, the following can happen:

1. connect is called
2. connectInternal is called and it works - reconnectAttempt is set to 0 in onReady
3. reconnectAttempt is increased
4. next loop iterations with unsuccessful connects

The starting value will be reconnectAttempt = 1, so effectively we will try to reconnect only AMQP_RECONNECT_MAX_ATTEMPTS - 1 times.

I think the better way is a minor refactoring of AmqpManager::connect() to wait before connecting:

void AmqpManager::connect() {
  this->connectInternal();
  while (this->reconnectAttempt < AMQP_RECONNECT_MAX_ATTEMPTS) {
    this->reconnectAttempt++;
    LOG(INFO) << "AMQP: Attempt " << this->reconnectAttempt
              << " to reconnect in " << AMQP_RECONNECT_ATTEMPT_INTERVAL_MS
              << " ms";
    std::this_thread::sleep_for(
        std::chrono::milliseconds(AMQP_RECONNECT_ATTEMPT_INTERVAL_MS));
    this->connectInternal();
  }
  LOG(FATAL) << "Cannot connect to AMQP server after "
             << AMQP_RECONNECT_MAX_ATTEMPTS << " attempts";
}

I've made an update with this.