[SERVER-48733] Update expectedPrimaryNodeId parameter in awaitNodesAgreeOnPrimary Created: 11/Jun/20 Updated: 29/Oct/23 Resolved: 16/Jul/20 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | None |
| Affects Version/s: | None |
| Fix Version/s: | 4.7.0 |
| Type: | Bug | Priority: | Major - P3 |
| Reporter: | Ali Mir | Assignee: | Ali Mir |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | neweng | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||
| Backwards Compatibility: | Fully Compatible | ||||
| Operating System: | ALL | ||||
| Sprint: | Repl 2020-07-13, Repl 2020-07-27 | ||||
| Participants: | |||||
| Linked BF Score: | 18 | ||||
| Description |
|
In the function awaitNodesAgreeOnPrimary in replsettest.js, we pass in a parameter expectedPrimaryNodeId. This refers to the node we expect to become primary. However, we use this parameter to index into the list of nodes passed in, and so it is misleading to name this variable as the nodeId when in fact it is the index of the expected primary in the nodes list parameter. Here is an instance in the codebase where we are misusing this parameter. We try to pass in 1 as the expectedPrimaryNodeId, as we expect node1 to become primary (shown in the previous line), but because we index into the passed in nodes list, we set the expected primary to be node 2. A possible solution to this problem would be to change the parameter in awaitNodesAgreeOnPrimary to actually take in a unique nodeId. There are only a few places in the codebase where we use this third parameter, so changing it should cause minimal disruption to existing tests. |
| Comments |
| Comment by Githook User [ 16/Jul/20 ] |
|
Author: {'name': 'Ali Mir', 'email': 'ali.mir@mongodb.com', 'username': 'ali-mir'}Message: |
| Comment by Ali Mir [ 15/Jun/20 ] |
|
siyuan.zhou Done. |
| Comment by Siyuan Zhou [ 15/Jun/20 ] |
|
As discussed in triage meeting, the issue isn't using node id as node index, but using the node id as the index in the given nodes arguments. The proposed solution still makes sense though. ali.mir, could you please link the BF ticket to this? This issue isn't the root cause of the BF, so I don't see the necessity of a quick fix on the misleading usage as long as it doesn't cause failures. |
| Comment by Judah Schvimer [ 11/Jun/20 ] |
|
ReplSetTest refers to node indexes as nodeId pretty frequently unfortunately, we may want a more consistent fix, or to just fix the one place that's wrong. |