search

globalsign / mgo

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

Changestreams db and cluster level #251 #258

Closed peterdeka closed 6 years ago

peterdeka commented 6 years ago

Enhancing change streams in order to support DB level and cluster level changes notifications introduced in mongoDB 4.0 as per issue #251. I strongly ask review of the way i addressed the question as i'm not really sure it is not a workaround. If we go this way, i think it's better to make unexported all session and database methods that were added (Pipe(), NewIter()), except for the Watch() method in order to not pollute with meaningless methods. Tests are still missing and will be added once we agree on the way i implemented the functionality and Mongo 4.0 is added to the test suite.

peterdeka commented 6 years ago

any news on this @eminano ? i just need a fast verification if my solution is viable in order to consolidate the PR completing the missing points.

Thanks, Pietro

eminano commented 6 years ago

Hi @peterdeka,

Sorry for the delay, it's been a busy week. Will review this PR between today and tomorrow!

Thanks, Esther

eminano commented 6 years ago

Hi @peterdeka,

As @domodwyer mentioned on issue #251 this looks like a reasonable approach for the implementation. Will wait for you to complete the PR before doing a full review.

Thanks! Esther

peterdeka commented 6 years ago

For me this is ok, i'm going on with the tests. But then we will need 4.0 to be in the suite to make sense of them.

domodwyer commented 6 years ago

Hi @peterdeka

If you have a branch with working cluster-level changestreams, just change the .travis.yml file in your PR to include 4.0 and it'll automatically run!

Dom

peterdeka commented 6 years ago

Thanks @domodwyer but i saw that as per #259 that has been already tried and was failing!

peterdeka commented 6 years ago

@eminano i'm good to go, Travis tests are obviously excluding 4.0 but i ran locally and they are green.

eminano commented 6 years ago

@peterdeka, could you please resolve the conflicts? We'll review once the build is ready.

Thanks! Esther

peterdeka commented 6 years ago

Just for the history books, currently blocked by #266

domodwyer commented 6 years ago

@peterdeka this is a really, really good PR - thanks so much for the contribution!

I left a single comment regarding the visibility of ChangeDomainType - I see no reason this can't be merged functionally!

Thanks very much - I know the mgo code base can be difficult to work with but this is a great PR.

Dom

eminano commented 6 years ago

Thanks @peterdeka for a great PR!