refactor: remove cursorState from Cursor and wire protcol methods #2549
Conversation
mbroadst
force-pushed
the
NODE-2810/remove-cursor-state
branch
from
a57f04d
to
d3505b9
Compare
Cursors presently have an internal state tracking object called `cursorState` which has a high degree of coupling with multiple components in the system, and is source of great confusion and bugs. This patch removes the `cursorState` completely, mounting its relevant properties directly on the `CoreCursor` type and removing it from the signature of the `query`, `killCursors`, and `getMore` wire protocol methods. Many changes were required as part of this refactorinig, but most notable is cleanup around cursor `close`. The internal call to kill the cursor raced with the `_endsession` method, which uncovered a number of failures in our test suite when fixed. NODE-2810
mbroadst
force-pushed
the
NODE-2810/remove-cursor-state
branch
from
d3505b9
to
2a42773
Compare
|
|
||
| if (typeof cursorState.session === 'object') { | ||
| options.session = cursorState.session; |
There was a problem hiding this comment.
Are we sure we want to stop passing the session here?
There was a problem hiding this comment.
Hey, sorry it looks like we lost your comments somewhere (maybe the force push). So what's going on here is that the wire protocol killCursors command used to take in a cursorState which might have had a session, and now it takes an options which may have a session. The options are passed all the way down to the command method which will use the session there, so there's no need to copy it here any longer
| : false; | ||
|
|
||
| // get rid of these? | ||
| this._limit = limit; |
There was a problem hiding this comment.
_limit and _batchSize seem to be used. What's the purpose of this comment and the deprecation comment above? Are these meant to be private? If so can we use private keyword?
There was a problem hiding this comment.
They are used, but won't be for much longe (referring to the cursor prototype I discussed in our meeting yesterday). For now, I wanted to keep these around to make @emadum 's work of combining Cursor and CoreCursor easier.
Cursors presently have an internal state tracking object called
cursorStatewhich has a high degree of coupling with multiple components in the system, and is source of great confusion and bugs. This patch removes thecursorStatecompletely, mounting its relevant properties directly on theCoreCursortype and removing it from the signature of thequery,killCursors, andgetMorewire protocol methods.Many changes were required as part of this refactoring, but most notable is cleanup around cursor
close. The internal call to kill the cursor raced with the_endsessionmethod, which uncovered a number of failures in our test suite when fixed.NODE-2810
NOTE: this is based on #2546 , so it will appear to have the same changes until that PR is merged and this one is rebased.