Repository navigation
fix found issues - #806
fix found issues#806
Conversation
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #806 +/- ##
==========================================
- Coverage 56.51% 55.27% -1.24%
==========================================
Files 108 110 +2
Lines 10300 13257 +2957
Branches 1402 1931 +529
==========================================
+ Hits 5821 7328 +1507
- Misses 3894 5084 +1190
- Partials 585 845 +260 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| void RaftReplDev::become_leader_cb() { | ||
| auto current_gate = m_traffic_ready_lsn.load(); | ||
| auto new_gate = raft_server()->get_last_log_idx(); | ||
| repl_lsn_t existing_gate = 0; |
There was a problem hiding this comment.
Could we add a assert here to confirm the new gate is larger or equal to the current one?
| m_traffic_ready_lsn.store(0); | ||
| RD_LOGD(NO_TRACE_ID, "become_follower_cb setting traffic_ready_lsn to 0"); | ||
| } | ||
| void become_follower_cb() { RD_LOGD(NO_TRACE_ID, "become_follower_cb called!"); } |
There was a problem hiding this comment.
Can we add m_traffic_ready_lsn.store(0) to make sure the follower can process the coming traffic
There was a problem hiding this comment.
I don`t get this. follower can not process any traffic since we have a check like this
if (!repl_dev->is_leader()) {
LOGW("failed to create shard for pg={}, not leader", pg_owner);
decr_pending_request_num();
return folly::makeUnexpected(ShardError(ShardErrorCode::NOT_LEADER, repl_dev->get_leader_id()));
}
if (!repl_dev->is_ready_for_traffic()) {
LOGW("failed to create shard for pg={}, not ready for traffic", pg_owner);
decr_pending_request_num();
return folly::makeUnexpected(ShardError(ShardErrorCode::RETRY_REQUEST));
}
m_traffic_ready_lsn is only used by leader , no?
There was a problem hiding this comment.
is_ready_for_traffic is not limited to the put path; resetting it ensures all operations, such as the get path, proceed as expected. For example, the get blob path: https://github.com/eBay/HomeObject/blob/main/src/lib/homestore_backend/hs_blob_manager.cpp#L297-L302
There was a problem hiding this comment.
I think the right solution is adding is_leader() check in get_blob path. we should always read_blob from leader, since follower is probably slow than leader , and return a wrong result. changing m_traffic_ready_lsn at follower side give no help? for example, if a put_blob has a lsn 100, and it is committed at leader, then when we get_blob at leader , we can find this blob. however, if we get_blob at follower , even if we have already set m_traffic_ready_lsn.store(0) at follower, which makes sure we pass this check (if (!repl_dev->is_ready_for_traffic())), lsn 100 might not be committed at follower and thus return a not_found for this get_blob at follower. so , I think follower should return not_leader here and let the client retry leader.
There was a problem hiding this comment.
IIRC, the gateway can read blobs from all members, with a policy to choose the member in the same DC to reduce latency. Reading blobs only from the leader increases the leader's load, so I believe allowing followers to support reads is the right strategy. If a follower cannot find the blob (e.g., due to uncommitted), the gateway will retry with other members
There was a problem hiding this comment.
I got your point, locality for reading is necessary.
I am thinking if the blob we are trying to read has been deleted and gc(we can not find the blob in pg_index_table), then every member will return UNKNOWN_BLOB error to gateway. what should gateway do?
retry for ever ? or there is any specific retry policy
there should be a guy to tell gateway that the blob does not exist in all members.
3 participants
1 in nuraft, a leader might become a new leader directly, we need adapt to this case.
2 for the error happens in on_fetch_data_received( in originator), do not assert, just ignore this request and let follower retry.
3 when stopping raft_repl_service, we should stop the reaper thread first so that fetch data will not happen after the repl_dev is destroyed.