[SERVER-46800] Fix doReplSetReconfig to always return a status on error Created: 11/Mar/20 Updated: 17/Mar/20 Resolved: 17/Mar/20 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | Replication |
| Affects Version/s: | None |
| Fix Version/s: | None |
| Type: | Bug | Priority: | Major - P3 |
| Reporter: | Vesselina Ratcheva (Inactive) | Assignee: | William Schultz (Inactive) |
| Resolution: | Done | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||
| Operating System: | ALL | ||||||||
| Sprint: | Repl 2020-03-23 | ||||||||
| Participants: | |||||||||
| Description |
|
The contract for doReplSetReconfig (as observed by all callers) is that it should always return a bad status on error and never throw (no callers catch exceptions). There is an exception to this when waiting for the config to be replicated to a majority (here) - that should be changed to a status return. |
| Comments |
| Comment by William Schultz (Inactive) [ 17/Mar/20 ] |
|
This should be addressed by the changes from |
| Comment by Judah Schvimer [ 11/Mar/20 ] |
|
I agree throwing an exception can be preferable to a status. I am wary of functions that do both though and it currently can return a status. |
| Comment by Siyuan Zhou [ 11/Mar/20 ] |
|
I agree with william.schultz that this will be addressed in We prefer to throw an exception on errors for user commands, but it seems doReplSetReconfig() is also called internally, for example on step up. Althought the code linked by Vessy isn't called outside of commands, it's a good idea to keep the contract consistent. |
| Comment by Judah Schvimer [ 11/Mar/20 ] |
|
siyuan.zhou, does this pose problems for safe reconfig? Does it need to be backported? |
| Comment by William Schultz (Inactive) [ 11/Mar/20 ] |
|
vesselina.ratcheva I think this will end up being addressed by |