Add get_leader_id/get_replication_status into replicationService #294
Conversation
| /// @brief get replication status. If called on follower member | ||
| /// this API can return empty result. | ||
| virtual std::vector<peer_info> get_replication_status(group_id_t group_id) const = 0; | ||
|
|
There was a problem hiding this comment.
Is there a use case where we have this api in ReplicationService or can this be at every ReplDev itself, because we have ReplDev::is_leader() already and we could do ReplDev::get_leader() and get_replication_status() there right?
There was a problem hiding this comment.
Yes there is no strong requirement to have that in ReplicationService as in ReplicationService it also get the ReplDev and operate based on that.. was trying to have a wrapper so that client(external HS) doesnt need to get ReplDev to get the stats when they want to report metrics. However in HO we already keep the ReplDev ptr in HS_PG.
Will move those to ReplDev.
xiaoxichen
force-pushed
the
xiaoxi_dev
branch
from
5a480f0 to
17c3b90
Compare
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #294 +/- ##
==========================================
+ Coverage 58.79% 58.88% +0.09%
==========================================
Files 110 110
Lines 9249 9258 +9
Branches 1190 1189 -1
==========================================
+ Hits 5438 5452 +14
+ Misses 3286 3278 -8
- Partials 525 528 +3 ☔ View full report in Codecov by Sentry. |
xiaoxichen
force-pushed
the
xiaoxi_dev
branch
2 times, most recently
from
8af95cf to
df96a06
Compare
| @@ -65,6 +65,15 @@ using remote_blkid_list_t = folly::small_vector< RemoteBlkId, 4 >; | |||
| using replica_id_t = uuid_t; | |||
| using group_id_t = uuid_t; | |||
|
|
|||
| struct peer_info { | |||
There was a problem hiding this comment.
is there a reason we can't use the same structure defined in nuraft_mes: https://github.com/eBay/nuraft_mesg/blob/main/include/nuraft_mesg/mesg_state_mgr.hpp#L31
There was a problem hiding this comment.
We are not exposing nuraft_mesg to the upper layer, so we probably have to redefine here.
There was a problem hiding this comment.
My thinking is althrough nuraft is our only serious implementation right now but as a generic API for all repl-dev I am trying to use more neutral structure/naming.
- the structure field (mostly
last_log_idx_) is too implementation(raft) specific - HS/HO we use replica_id_t, translate it from string to replica_id_t make it more consistent from the view of HO.
| @@ -12,6 +12,7 @@ | |||
| #include "replication/service/raft_repl_service.h" | |||
| #include "replication/repl_dev/raft_repl_dev.h" | |||
| #include "push_data_rpc_generated.h" | |||
| #include "boost/lexical_cast.hpp" | |||
There was a problem hiding this comment.
nit: It is not mandatory, but common practice we decided to follow was to define the
STL defs, followed by external libraries both indicate by <> (instead of "") and then followed by local repo includes.
Signed-off-by: Xiaoxi Chen <[email protected]>
xiaoxichen
force-pushed
the
xiaoxi_dev
branch
from
b77e371 to
fd29422
Compare
No description provided.