Simplify isNodeAvailable function and add comments #994
Conversation
Use getNodeReplicationOffset to replace the original logic and add the corresponding annotations. Signed-off-by: Binbin <binloveplay1314@qq.com>
| @@ -1332,16 +1332,18 @@ void addNodeToNodeReply(client *c, clusterNode *node) { | |||
| * not finished their initial sync, in failed state, or are | |||
| * otherwise considered not available to serve read commands. */ | |||
| int isNodeAvailable(clusterNode *node) { | |||
| /* We don't consider PFAIL here because it's not a reliable indicator | |||
| * for node available and we don't want clients to use it. */ | |||
There was a problem hiding this comment.
I know that we rejected pfail in the past, but i can add a point of view. From myself own perspective, if a node is pfail, it means that it is unavailable from myself view and we can mark it unavailable in myself response.
There was a problem hiding this comment.
I don't quite follow your comment. There isn't much harm in removing a temporarily unavailable node, but showing a dead node to clients is bad since they will timeout connecting to it.
There was a problem hiding this comment.
I think what @enjoy-binbin meant here is PFAIL state is not taken into consideration as it's just a self view and not yet confirmed by the quorum.
There was a problem hiding this comment.
yes, hpatro saying is right. Sorry about the confuse, bad english.
there are two comments, the one in the code, is somehow i took from the old PR, maybe the CLUSTER SHARDS one, back then, we think a pfail node is available, so in here, we only skip fail node, the clusterNodeIsFailing one.
and in here, the comment i added as a github review comment, is saying, a pfail node may be unavailable in myself's view. The decision we made in the past in cluster shard was that the pfail node was available so we won't skip it. But if we look at it from myself own perspective, if a node pfails, can we assume that myself think it is unavailable?
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## unstable #994 +/- ##
============================================
- Coverage 70.57% 70.56% -0.01%
============================================
Files 114 114
Lines 61634 61630 -4
============================================
- Hits 43498 43492 -6
- Misses 18136 18138 +2
|
| @@ -1332,16 +1332,18 @@ void addNodeToNodeReply(client *c, clusterNode *node) { | |||
| * not finished their initial sync, in failed state, or are | |||
| * otherwise considered not available to serve read commands. */ | |||
| int isNodeAvailable(clusterNode *node) { | |||
| /* We don't consider PFAIL here because it's not a reliable indicator | |||
| * for node available and we don't want clients to use it. */ | |||
There was a problem hiding this comment.
I think what @enjoy-binbin meant here is PFAIL state is not taken into consideration as it's just a self view and not yet confirmed by the quorum.
Co-authored-by: Harkrishn Patro <bunty.hari@gmail.com> Signed-off-by: Binbin <binloveplay1314@qq.com>
Use getNodeReplicationOffset to replace the original logic
and add the corresponding annotations.