[SERVER-38831] Make the cursor option to the aggregate command optional again Created: 03/Jan/19 Updated: 06/Dec/22 |
|
| Status: | Backlog |
| Project: | Core Server |
| Component/s: | Aggregation Framework |
| Affects Version/s: | None |
| Fix Version/s: | None |
| Type: | Improvement | Priority: | Major - P3 |
| Reporter: | Charlie Swanson | Assignee: | Backlog - Query Execution |
| Resolution: | Unresolved | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||
| Assigned Teams: |
Query Execution
|
||||||||
| Participants: | |||||||||
| Description |
|
This is often forgotten in many of our tests causing pain during development. Now that it's been a number of releases since we've supported the command without the cursor option, we should make the cursor behavior the default and not require cursor: {} to be specified on each aggregate command. |
| Comments |
| Comment by Charlie Swanson [ 11/Jan/19 ] |
|
scott.lhommedieu I'm not sure I totally understand your concern but the reason we changed the response format was to enable result sets larger than 16MB to be returned from an aggregation. |
| Comment by Charlie Swanson [ 11/Jan/19 ] |
|
It is just convenient. behackett I'm surprised we care about the quality of an error message a user would receive in that scenario. Such a user would have to have a very old version of a driver lying around (older than whatever the first version of the driver was to default to using cursors for aggregations. I would expect this to be somewhere around when that functionality was introduced which was 2.6. At the very least that functionality was deprecated in 3.4 which was what, 3.5 years ago?) and try to use that on a brand new version of the server (4.2, which won't be released until this summer-ish). In contrast, I'd expect most people either download a brand new version of both, or are upgrading some old version of both. Admittedly, this is just a hunch If we aren't comfortable doing this now, will we ever be? |
| Comment by Scott L'Hommedieu (Inactive) [ 11/Jan/19 ] |
|
This seems like a bad user experience across different versions of the driver in terms of aggregate having changing response formats without a change in the function api AND no clear error. Why do this? What is the user gaining from this change in response format (even if it is only to enable other features). |
| Comment by Bernie Hackett [ 11/Jan/19 ] |
|
I have mixed feelings about this. Old drivers did indeed previously break. But the error message for that was very clear. They will still break but there will be no error message at all. The driver will just fail in a way that is difficult for users to understand. Is this change really critical, or just convenient? |
| Comment by Andy Schwerin [ 10/Jan/19 ] |
|
OK with me. Get a driver dev to double-check your reasoning, though. |
| Comment by Charlie Swanson [ 10/Jan/19 ] |
|
schwerin and scott.lhommedieu we discussed this in the query team's triage meeting today. bernard.gorman pointed out that on 4.0, an old driver connecting to the server and not specifying the cursor option would fail, so this combination is already unsupported. If we changed it so that the cursor option is no longer required in 4.2 that same old driver would have the command succeed, but the response would come back in a format it's likely not anticipating. Namely, the cursor response format, something like {cursor: {id: XXX, firstBatch: [...]}} instead of just {results: [...]}. Given this, I'm pretty comfortable making such a change on the 4.2 branch, and thus we've marked this as a nomination for Q1 quick wins. The main win here would be saving developers time writing tests or running raw aggregate commands in the shell. cc david.storch and asya. |
| Comment by Andy Schwerin [ 04/Jan/19 ] |
|
Will this impact old drivers connecting to new servers? scott.lhommedieu |
| Comment by Charlie Swanson [ 03/Jan/19 ] |
|