-
Notifications
You must be signed in to change notification settings - Fork 600
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Simplify isNodeAvailable function and add comments #994
base: unstable
Are you sure you want to change the base?
Simplify isNodeAvailable function and add comments #994
Conversation
Use getNodeReplicationOffset to replace the original logic and add the corresponding annotations. Signed-off-by: Binbin <[email protected]>
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <[email protected]> Signed-off-by: Binbin <[email protected]>
Use getNodeReplicationOffset to replace the original logic
and add the corresponding annotations.