Add a cursor abstraction #1

jyemin · 2017-02-08T18:40:19Z

Added a cursor abstraction to core. Some notes:

Works only with 3.2 and up, as it relies on the getMore and killCursors commands
Added some common types to command_types.go. We may not want to keep these, but I needed them for both the tests and examples so currently they are public.
The Cursor abstraction has the same API as mgo
There's a missing abstraction around connection management. Currently NewCursor is passed a Connection, which is used for iterating the cursor with getMore and closing it with killCursors. This is fine for our purposes, since we're going to be using a single connection per session, but once we add connection pooling to the client we'll need to do this differently so that a connection can be retrieved from a pool.
No tailable support

…e collection does not exist

craiggwilson · 2017-02-08T20:15:16Z

core/command_types.go

@@ -0,0 +1,19 @@
+package core


We have a results.go file these can go into. I'd probably also create a CursorResult interface that has 3 methods, NS(), ID(), and FirstBatch()

craiggwilson · 2017-02-08T20:15:45Z

core/command_types.go

+	// The first batch of the cursor
+	FirstBatch []bson.Raw `bson:"firstBatch"`
+	// The namespace to use for iterating the cursor
+	NS string `bson:"ns"`


Let's create that Namespace struct type.

craiggwilson · 2017-02-08T20:16:47Z

core/cursor.go

+)
+
+// Create a new cursor
+func NewCursor(databaseName string, collectionName string, firstBatch []bson.Raw, cursorId int64, batchSize int32, connection Connection) Cursor {


Once you have a CursorResult interface, this should just be able to take that as an argument for the first 4 parameters.

craiggwilson · 2017-02-08T20:24:11Z

core/cursor.go

+	c.currentBatch = response.Cursor.NextBatch
+}
+
+type getMoreCommand struct {


I don't necessarily see these types getting used anywhere outside this particular file. It'd be nice to define these types right where they will be used inside the functions. In some cases, they could probably even be anonymous structs.

For example:

getMoreCommand := struct { CursorId int64 `bson:"getMore"` Collection string `bson:"collection"` BatchSize int32 `bson:"batchSize"` } { CursorId: c.cursorId, Collection: c.collectionName, }

Same with the responses...

I could probably do this in other places too...

craiggwilson · 2017-02-08T20:26:15Z

core/cursor_test.go

+	"testing"
+)
+
+const collectionName = "CursorTest"


This collectionName variable is now available to all the things using the core_test package. Meaning, that this name is probably not what we want to be used in other packages that aren't testing cursors...

If we didn't use the same collection name for each test, we could add t.Parallel() to each of these.

Done, though didn't make the tests parallel yet

craiggwilson · 2017-02-08T20:31:31Z

core/cursor.go

+	currentBatch   []bson.Raw
+	cursorId       int64
+	err            error
+	connection     Connection // TODO: missing abstraction.  Shouldn't require a connection here, but just a way to acquire and release one


Let's have this take a Server instead...

Deferred, as discussed

craiggwilson

lgtm. Little nits and fix the example, but push when you are ready.

craiggwilson · 2017-02-08T21:59:31Z

examples/cursor/main.go

+		log.Fatalf("Failed to execute command: %v", err)
+	}
+
+	cursor := core.NewCursor(databaseName, collectionName, response.Cursor.FirstBatch, response.Cursor.ID, 0, connection)


seems like this isn't gonna work anymore.

craiggwilson · 2017-02-08T22:01:33Z

core/cursor_test.go

@@ -0,0 +1,199 @@
+package core_test


since you are using different collections, might as well mark each with t.Parallel().

Also, might be nice to find a way to get the current method name from a stack trace, then use that for the collection name?

Marked as parallel. Leaving the current method name detection for another time.

craiggwilson · 2017-02-08T22:02:06Z

core/cursor_test.go

+	"testing"
+)
+
+func TestEmpty(t *testing.T) {


Maybe let's prefix each of these with what it's testing... TestCursor_Empty, TestCursor_Close, TestCursor_SingleBatch... etc...

Done, though without the underscore separator

craiggwilson

lgtm. 2 debug comments left in.

craiggwilson · 2017-02-08T22:55:54Z

core/cursor.go

@@ -44,9 +45,11 @@ type cursorImpl struct {
 func (c *cursorImpl) Next(result interface{}) bool {
 	found := c.getNextFromCurrentBatch(result)
 	if found {
+		fmt.Println("found")


craiggwilson · 2017-02-08T22:55:59Z

core/cursor.go

 		return true
 	}
 	if c.err != nil {
+		fmt.Printf("error: %v\n", c.err)


jyemin · 2017-02-09T04:30:59Z

Rebasing and committing

merge

STITCH-4296: Export contextWithSession

GODRIVER-672 Change session IDs to be stored as bson.Raw (mongodb#339)

Add a cursor abstraction

119f309

jyemin requested review from craiggwilson and rychipman February 8, 2017 18:40

jyemin added the enhancement label Feb 8, 2017

jyemin added 3 commits February 8, 2017 14:22

Remove out-dated example

d77a0b2

gofmt

7f9113d

Support MongoDB 3.2, in which the drop command reports an error if th…

dbfcfd2

…e collection does not exist

craiggwilson requested changes Feb 8, 2017

View reviewed changes

Response to code review

2506c0f

craiggwilson reviewed Feb 8, 2017

View reviewed changes

Response to code review 2

5fee3da

craiggwilson approved these changes Feb 8, 2017

View reviewed changes

Code review comments 3

f0c6184

jyemin closed this Feb 9, 2017

skriptble pushed a commit that referenced this pull request Dec 6, 2018

Merge pull request #1 from mongodb/master

0d4a3e7

merge

edaniels referenced this pull request in edaniels/mongo-go-driver Feb 10, 2020

Merge pull request #1 from haleyowen/STITCH-4296

6d9a8ff

STITCH-4296: Export contextWithSession

edaniels referenced this pull request in mongodb-forks/mongo-go-driver Sep 1, 2020

Merge pull request #1 from jbreams/backport-session-id

104b50f

GODRIVER-672 Change session IDs to be stored as bson.Raw (mongodb#339)

Add a cursor abstraction #1

Add a cursor abstraction #1

Uh oh!

Conversation

jyemin commented Feb 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

craiggwilson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

craiggwilson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jyemin commented Feb 9, 2017

Uh oh!

Uh oh!

jyemin commented Feb 8, 2017 •

edited

Loading