globalsign / mgo

The MongoDB driver for Go
Other
1.97k stars 231 forks source link

Change session.DatabaseNames to use the `nameOnly` option (if available) #331

Closed jacksontj closed 5 years ago

jacksontj commented 5 years ago

listDatabases by default returns the list of database names as well as some stats about the database (size, etc.). To gather the stats mongod acquires a db level lock, which is very slow (and depending on other locks etc. may cause this to unnecessarily fail).

Perf comparison:

basic took 2.772775182s
nameonly took 93.680166ms

I have also tested that this additional option falls back to the previous behavior on mongo <3.6

Mongo docs: https://docs.mongodb.com/manual/reference/command/listDatabases/

jacksontj commented 5 years ago

cc @eminano @domodwyer

jacksontj commented 5 years ago

bump?

jacksontj commented 5 years ago

Any update @domodwyer @eminano

domodwyer commented 5 years ago

Hi @jacksontj

Your code seems to be failing tests? I've kicked off another build but pretty sure it's going to fail again - some of them are linter errors.

Would you mind changing this to point at development?

Dom

jacksontj commented 5 years ago

@domodwyer I've changed the merge target and fixed the linter errors. I missed them before, I had assumed linter checks would have been in all the builds, not just some (as there were errors only on so me builds). At this point I expect it to pass, so I'll keep an eye on that result.

jacksontj commented 5 years ago

@domodwyer CI passed :) When this is merged, what is the usual delay for getting to the main branch? This fix would be hugely helpful for the mongodb exporter (specifically when metrics are happening during an initial sync-- as right now it attemptes to get locks etc. and mongo unfortunately doesn't cancel locks when the connection closes).

jacksontj commented 5 years ago

@eminano Actually there are already tests that check the return of session.DatabaseNames() e.g. https://github.com/globalsign/mgo/blob/master/session_test.go#L570

So this should be all set for a merge (although it seems that your approval doesn't give me the merge button ;) )

eminano commented 5 years ago

@jacksontj, my bad, I was filtering out test files by mistake when I checked :D The merge is only allowed for members I'm afraid!

Thanks again for your contribution, it's really appreciated! Esther

jacksontj commented 5 years ago

@eminano thanks for getting this merged! How long does it usually take for patches like this to make it to master? I have some projects that could really use this patch, and I'd like to continue working of of master.

eminano commented 5 years ago

@jacksontj, ideally we'll be cutting a release within a months or so, just waiting to review some of the ongoing PRs that introduce some interesting features/improvements.