Leadership expiry not respected as a limit for leadership expiry

[antoniofilipovic]

Hi,

I expected the leadership_expiry_ flag to be used as the amount of time needed to pass between now and the last time the follower responded to the leader, for the leader to yield its leadership.

However, looking at the code, leadership only expires after followers are deemed unhealthy. And followers are deemed unhealthy according to the following policy:

int expiry = params->heart_beat_interval_ *
                     raft_server::raft_limits_.response_limit_;

    // Check the number of not responding peers.
    for (auto& entry: peers_) {
        ptr<peer> p = entry.second;

        if (!is_regular_member(p)) continue;

        int32 resp_elapsed_ms = (int32)(p->get_resp_timer_us() / 1000);
        if ( resp_elapsed_ms > expiry ) {
            num_not_resp_nodes++;
        }
    }

I would like to open PR to fix this issue that the expiry used in this function is the same as the leadership expiry time, as otherwise this flag is not used to calculate when will leadership expire.

Let me know if this is actually not an issue, and I didn't understand something properly.

many thanks,
Antonio

[greensky00]

Thanks for pointing this out. Looks like this part of code was previously correct, and then the bug has been introduced since the below commit.
bb863ab#diff-60ca73a04b9bb2ab5fa615dc2a38a817a8260975d6c06d35ca90a02555731635R573

As get_not_responding_peers_count and get_not_responding_peers are currently used for other purposes as well, can we put an optional parameter to set custom expiry? For example,

std::list<ptr<peer>> get_not_responding_peers(int expiry = 0);
size_t get_not_responding_peers_count(int expiry = 0);
void apply_to_not_responding_peers(const std::function<void(const ptr<peer>&)>& callback, int expiry);

If given expiry is non-zero, honor the given expiry. If not, use heart_beat_interval_ * reponse_limit_.

[antoniofilipovic]

Hi, @greensky00. Thanks for the fast response.

The change which you suggested makes sense. I will open the PR to fix it!