[SERVER-68076] Reconsider usage of optional<RecordId> Created: 15/Jul/22 Updated: 01/Dec/22 Resolved: 01/Dec/22 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | None |
| Affects Version/s: | None |
| Fix Version/s: | None |
| Type: | Task | Priority: | Major - P3 |
| Reporter: | Jordi Olivares Provencio | Assignee: | Jordi Olivares Provencio |
| Resolution: | Won't Do | Votes: | 0 |
| Labels: | techdebt | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||
| Sprint: | Execution Team 2022-12-12 | ||||||||
| Participants: | |||||||||
| Description |
|
RecordId contains an explicit Null state when created without any data. There are currently usages of RecordId in a boost::optional which means we need to double check the nullability of the given element. This ticket is about reconsidering usages of this in favour of a plain RecordId with the Null state to avoid unnecessary wrapping of RecordId into a boost::optional. |
| Comments |
| Comment by Jordi Olivares Provencio [ 01/Dec/22 ] |
|
After investigating the current usage I don't think this ticket should be done. The current optional<RecordId> usages are to semantically convey the idea that there might or might not be a RecordId inside. We could replace them with the RecordId Null state indeed, but once in that scenario people now need to know that a RecordId can be nullable and check anyways. As optional<RecordId> does this function with much lower mental overhead I wouldn't be comfortable removing its usage just for the sake of removing a technically unnecessary wrapper. Additionally, these usages are due to how we internally pass configuration to the internal methods, for example CollectionScanParams. All in all, the changes required by this aren't worthwhile. Closing the ticket as Won't Do. |