[SERVER-34947] libmongodb_capi errors when aggregating with $out Created: 11/May/18 Updated: 29/Oct/23 Resolved: 16/May/18 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | None |
| Affects Version/s: | None |
| Fix Version/s: | 4.0.0-rc0 |
| Type: | Task | Priority: | Major - P3 |
| Reporter: | Ross Lawley | Assignee: | Henrik Edin |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Backwards Compatibility: | Fully Compatible |
| Sprint: | Platforms 2018-05-21 |
| Participants: |
| Description |
|
Running the following command via the embedded java driver:
Throws a:
The server logging callback returns:
|
| Comments |
| Comment by Githook User [ 16/May/18 ] |
|
Author: {'email': 'henrik.edin@mongodb.com', 'username': 'henrikedin', 'name': 'Henrik Edin'}Message: $out is using it. |
| Comment by Henrik Edin [ 15/May/18 ] |
|
How about doing both? We enable getLastError to be safe and we fix this case where it seems not to be needed and we can instead check the result of insert. |
| Comment by Andrew Morrow (Inactive) [ 15/May/18 ] |
|
I'm fairly sure we should go with re-supporting getLastError in embedded. My reasoning is that it may be the case that $out isn't the only path that reaches into a getLastError call in some way. Rather than playing whack-a-mole, lets just support it. Next questions to my mind are whether the current implementation of the command drags in any unwanted dependencies, and, if so, should we invest in rolling an embedded specific version, as we do for isMaster |
| Comment by Charlie Swanson [ 15/May/18 ] |
|
I don't think we need to worry too much about preserving the particular error message or code here. Changing the code might involve changing some tests, but I don't think anyone does/should rely on either. As long as the error still details what went wrong that should be fine. Also - it looks like this is actually trying to create an index, but it does so by inserting into system.indexes... Perhaps there's a new/better way to do that? |
| Comment by Henrik Edin [ 15/May/18 ] |
|
Re-support getlasterror is trivial. But redbeard0531 suggested to instead look at the result of the insert instead of using getlasterror. We can for example expose an insertWithResult() on the DBClient but that would change the assert message here: I'm willing to do the change, or forward this ticket to Query. Unless we want to play it super safe and just restore getlasterror for embedded. |
| Comment by Andrew Morrow (Inactive) [ 11/May/18 ] |
|
Thanks charlie.swanson - We will give it some thought, but most likely the path of least resistance is to re-support getlasterror in this context. If this path reaches it, there are probably others that we haven't found yet. |
| Comment by Charlie Swanson [ 11/May/18 ] |
|
acm Check out this code. I'm unfamiliar with the various interfaces of the DBDirectClient, but if there's a better interface that returns an error we should probably use that. We could also consider a switch to a raw PlanExecutor or some other path to insert, but that has some broader implications, including changes to how an $out will appear in things like the profiler and the logs. |
| Comment by Andrew Morrow (Inactive) [ 11/May/18 ] |
|
Apparently, running with $out somehow invokes the getlasterror command, which mobile explicitly doesn't support. david.storch or charlie.swanson could we get some insight from query on this issue? Why is $out trying to invoke the getlasterror command? |