search

globalsign / mgo

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

Partial 4.0 Transaction support #280

Closed duskglow closed 4 years ago

duskglow commented 6 years ago

Third time's a charm! I think we did it right this time.

This time, I pulled the latest version of development and ported my changes into that. This contains the following changes:

1) Partial 4.0 transaction support. This supports the basic Update, Upsert, Remove, and Insert support. Batch support is not included (but could probably easily be added - it's not a use case we needed at the moment). 2) dbtest supports creating a replica set now. You have to explicitly enable that. 3) A few other minor bugs and issues fixed.

Documentation is included in the repo.

All changes are, to the best of my knowledge, backwards compatible. All tests pass for me with the exception of some clustering tests that may be related to 4.0 but are not related to my changes to the best of my knowledge.

We at The Home Depot are excited to be sharing these changes with the community, please let me know if there are further issues that need to be addressed.

eminano commented 6 years ago

Hi @duskglow,

The build is failing due to tests you added in this PR, could you please have a look at those? "Transaction numbers are only allowed on a replica set member or mongos"

Thanks again! Esther

duskglow commented 6 years ago

I can look. But again, those tests are all passing for me in my environment. Do the 4.0 clusters in travis ci have replica sets enabled? Transactions will always fail under 4.0 unless it's a replica set cluster.

eminano commented 6 years ago

Transactions will always fail under 4.0 unless it's a replica set cluster

@duskglow I feel like that should be somehow reflected either on the tests or the code. It doesn't justify failing tests :)

duskglow commented 6 years ago

I disagree. If it is the tests that are failing because either the tests or the code are wrong, then I will be the first with a mea culpa. I am suggesting both the tests and the code are correct, but that the underlying infrastructure is not configured properly for the tests to succeed.

Of course, I am open to suggestions in how to have the tests test transactions when the infrastructure doesn't support them.

Meantime I will double check that I havent done something on my side. Error on my part is always a possibility.

eminano commented 6 years ago

@duskglow I think you misunderstood my last comment.

I meant either the code should have a restriction to prevent using transactions if it's a replica set cluster or the tests should cover for that scenario by for example skipping it in the expected failing cases. You can't guarantee your tests will always run in a cluster that satisfies your test success conditions, so if there's a limitation it should be handled somehow.

Hope this helps clarify, Esther

domodwyer commented 6 years ago

Hi @duskglow

Of course, I am open to suggestions in how to have the tests test transactions when the infrastructure doesn't support them.

To be clear, the test infrastructure is part of this repository - if it is unsuitable to test your new contributions in its current form please do change them to suit your needs :)

I hope you understand we cannot merge code that is failing tests and break the pipeline, regardless of the cause!

Dom

duskglow commented 6 years ago

It does clarify. I did not realize the test infrastructure is under my control. I do not understand why my tests are passing but failing when it hits travis ci, though. It will be hard to troubleshoot that as I cannot yet replicate the problem.

Your position is clearer now, thanks. It will be a little while before I can send a corrected patch, as not only is there some troubleshooting to do but we also have internal deadlines to meet. Nonetheless, I'll try to have a fix shorrly.

eminano commented 5 years ago

Hi @duskglow,

Any luck fixing those tests? If you don't have the time to do it we can always close this PR, and you can reopen it whenever it's convenient again.

This is a very useful PR and it would be great to have it merged, so thanks a lot for the time and effort! :)

Cheers, Esther

duskglow commented 5 years ago

Hi, thanks for the followup. Our project using these transactions has been under heavy active development, and I haven't had the time to go back and fix this pull request. I'll try to get some time in our sprint to get these problems ironed out in the next few weeks.

I really want to have it merged as well, so I'm not giving up on it quite yet!

duskglow commented 5 years ago

I'm poking at this now and have been making some progress. I see that there is another pull request to handle this in a different way. I'm not sure which way is better, honestly, but at least it passes all of its tests. Let me know how you want to proceed.

duskglow commented 4 years ago

I'm closing this PR. Another one was accepted for this functionality (and I think rightly so), and I coded around the reason I needed transactions in the first place. Thanks for your patience, anyway.