Notify user code about follower loss #517

apostolpav113 · 2024-06-18T04:45:33Z

Hello, within this PR we suggest to introduce a new cb_func::Type enum item: FollowerLost, and, on detection of follower loss, call the callback function with the type parameter equal to FollowerLost.

Pavel Yurin,
C++ developer
NXLog Ltd.
[email protected]
www.nxlog.co

greensky00

Thanks for submitting PR. I put a few comments, please take a look.

greensky00 · 2024-06-18T23:19:51Z

include/libnuraft/callback.hxx

+         * the peer is considered lost.
+         * ctx: null.
+         */
+        FollowerLost = 100,


If there is no other special reason, please use 28.

greensky00 · 2024-06-18T23:20:14Z

include/libnuraft/peer.hxx

+    bool IsLost() const { return lost_by_leader_; }
+    void SetLost() { lost_by_leader_ = true; }
+    void SetRecovered() { lost_by_leader_ = false; }


Please follow the naming convention: is_lost, set_lost, and set_recovered.

greensky00 · 2024-06-18T23:44:27Z

src/raft_server.cxx

+    const auto expiry =
+        params->heart_beat_interval_ * raft_server::raft_limits_.response_limit_;
+    std::vector<ptr<peer>> rs;
+    for (const auto& [id, peer_ptr]: peers_) {


It is a c++17 feature, and this project is based on c++11. Please use const auto& p and p.second. Looks like id is not used.

Yes, my bad, sorry. Fixed.

greensky00 · 2024-06-18T23:48:08Z

src/raft_server.cxx

@@ -837,6 +863,9 @@ void raft_server::handle_peer_resp(ptr<resp_msg>& resp, ptr<rpc_exception>& err)
            peer* pp = entry->second.get();
            int rpc_errs = pp->get_rpc_errs();
            if (rpc_errs >= raft_server::raft_limits_.warning_limit_) {
+                if (pp) {


Since the flow enters here only when entry != peers.end(), this if (pp) is unnecessary.

It doesn't guarantee that a shared pointer has a value, does it? But I see that you don't check this at the line above (pp->get_rpc_errs()), so ok, I removed this if (pp) here as well

greensky00 · 2024-06-18T23:54:19Z

src/raft_server.cxx

+        for (auto& peer : nr_peers_list) {
+          if (peer->IsLost()) {
+            continue;
+          }
+          peer->SetLost();
+          cb_func::Param param(id_, leader_, peer->get_id());
+          const auto rc = ctx_->cb_func_.call(cb_func::FollowerLost, &param);
+          assert(rc == cb_func::ReturnCode::Ok);
+        }


Please use 4-space indent.

greensky00 · 2024-06-18T23:56:21Z

src/raft_server.cxx

@@ -1085,7 +1114,7 @@ bool raft_server::check_leadership_validity() {
    int32 num_voting_members = get_num_voting_members();

    int leadership_expiry = get_leadership_expiry();
-    int32 nr_peers = (int32)get_not_responding_peers();
+    int32 nr_peers = (int32)get_not_responding_peers_count();


Since get_not_responding_peers is also used in the same function, can we just call it once?

const auto nr_peers_list = get_not_responding_peers(); int32 nr_peers = nr_peers_list.size();

Regarding this point:

on one hand, yes nothing prevents us from using get_not_responding_peers() once;

but on another hand, this way we can get an overhead: creating an std::list, filling it up with ptr<peer>s, returning it (though, RVO should work here) - and we need the list only in the case if the condition in the if below is true ((num_voting_members - nr_peers < min_quorum_size) == true), in all other cases we don't need the list; I assume that in the majority of cases, in "normal" circumstances, that condition should be false

But if you are ok with this overhead, plese let me know and I will change the code

Also, please pay attention, I have changed the code of the get_not_responding_peers_count/get_not_responding_peers functions. They contained a duplicated part of code, which could potentially lead to some bugs in the future (if someone change the code of one function, but not another). Now I extracted that common part into a separate function.

Good point, your change looks good.

greensky00 · 2024-06-18T23:58:53Z

src/raft_server.cxx

@@ -608,7 +608,25 @@ int32 raft_server::get_leadership_expiry() {
    return expiry;
 }

-size_t raft_server::get_not_responding_peers() {
+std::vector<ptr<peer>> raft_server::get_not_responding_peers() {


It seems to me std::list<...> is enough for the purpose of this function.

Ok, changed to std::list

…dness"

apostolpav113 · 2024-06-26T05:36:50Z

Thanks for submitting PR. I put a few comments, please take a look.

@greensky00, thank you for the review. I have made the fixes, and regarding this comment #517 (comment) - need one more round of discussion. Please take a look

greensky00

Thanks, another couple of comments.

greensky00 · 2024-07-02T06:32:57Z

src/raft_server.cxx

@@ -1085,7 +1114,7 @@ bool raft_server::check_leadership_validity() {
    int32 num_voting_members = get_num_voting_members();

    int leadership_expiry = get_leadership_expiry();
-    int32 nr_peers = (int32)get_not_responding_peers();
+    int32 nr_peers = (int32)get_not_responding_peers_count();


Good point, your change looks good.

greensky00 · 2024-07-02T06:45:51Z

include/libnuraft/peer.hxx

@@ -498,6 +502,8 @@ private:
     */
    std::atomic<bool> abandoned_;

+    std::atomic<bool> lost_by_leader_ {false};


Please add comment about this member variable.

Also, instead of brace initializer here, please put the default value into constructor, to be aligned with others.

greensky00 · 2024-07-02T06:47:56Z

src/raft_server.cxx

@@ -837,6 +861,7 @@ void raft_server::handle_peer_resp(ptr<resp_msg>& resp, ptr<rpc_exception>& err)
            peer* pp = entry->second.get();
            int rpc_errs = pp->get_rpc_errs();
            if (rpc_errs >= raft_server::raft_limits_.warning_limit_) {
+                pp->set_recovered();


set_lost() is invoked in two places:

handle_peer_resp: based on raft_limits_.warning_limit_, and

check_leadership_validity: from get_not_responding_peers which is based on raft_limits_.response_limit_.

Although both limits have the same default value, they can be configured with different numbers. If warning_limit_ > response_limit_, there can be a possibility that set_recovered() may not be called, when a leader loses its role and then becomes leader later again.

Please confirm this. I think calling set_recovered outside of this if-clause is ok. It is just setting an atomic variable.

On second thought, calling set_recovered for all peers when a server becomes leader (in raft_server::become_leader) is also an option. Leader may need a fresh start.

greensky00 · 2024-07-18T23:30:09Z

@apostolpav113
Since it has been pending for a while, I made those changes. Please let me know if you have any concerns. You can push follow-up PRs anytime.

apostolpav113 · 2024-08-08T08:40:00Z

greensky00, sorry for delaying and thank you for making the changes! :-) I was going to start fixing today, and was surprised by the fact that PR had been already merged

Notify user code about follower loss

ab777fc

greensky00 reviewed Jun 19, 2024

View reviewed changes

Pavel Yurin added 2 commits June 26, 2024 13:06

PR review fixes

e0400a5

Fixed a warning "comparison of integer expressions of different signe…

b2d0b07

…dness"

greensky00 reviewed Jul 2, 2024

View reviewed changes

[Update PR] Add comments and put more set_recovered() calls

e45952a

greensky00 approved these changes Jul 18, 2024

View reviewed changes

Merge branch 'master' into Follower-loss-notification

480b1ad

greensky00 merged commit 0e28077 into eBay:master Jul 18, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Notify user code about follower loss #517

Notify user code about follower loss #517

apostolpav113 commented Jun 18, 2024

greensky00 left a comment

greensky00 Jun 18, 2024

apostolpav113 Jun 26, 2024

greensky00 Jun 18, 2024

apostolpav113 Jun 26, 2024

greensky00 Jun 18, 2024

apostolpav113 Jun 26, 2024

greensky00 Jun 18, 2024

apostolpav113 Jun 26, 2024

greensky00 Jun 18, 2024

apostolpav113 Jun 26, 2024

greensky00 Jun 18, 2024

apostolpav113 Jun 26, 2024

greensky00 Jul 2, 2024

greensky00 Jun 18, 2024

apostolpav113 Jun 26, 2024

apostolpav113 commented Jun 26, 2024 •

edited

Loading

greensky00 left a comment

greensky00 Jul 2, 2024

greensky00 Jul 2, 2024

greensky00 Jul 2, 2024

greensky00 Jul 2, 2024

greensky00 commented Jul 18, 2024

apostolpav113 commented Aug 8, 2024

Notify user code about follower loss #517

Notify user code about follower loss #517

Conversation

apostolpav113 commented Jun 18, 2024

greensky00 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apostolpav113 commented Jun 26, 2024 • edited Loading

greensky00 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greensky00 commented Jul 18, 2024

apostolpav113 commented Aug 8, 2024

apostolpav113 commented Jun 26, 2024 •

edited

Loading