Skip to content

Conversation

@kmahar
Copy link
Contributor

@kmahar kmahar commented Apr 7, 2020

This PR updates our client initializer to accept an EventLoopGroupProvider rather than an EventLoopGroup.
As a result I needed to update our shutdown API since it can no longer return a future.

I took a lot of inspiration writing shutdown code from:

Comments inline

@kmahar kmahar requested review from mbroadst and patrickfreed April 7, 2020 21:21
@kmahar
Copy link
Contributor Author

kmahar commented Apr 7, 2020

also all this definitely necessitates updating guides/examples but I figured I'd see what you all have to say on the API before I do that

Copy link
Contributor

@patrickfreed patrickfreed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API looks good, great job on researching what the current standards are. I just a have a few comments about the internals.

@kmahar kmahar force-pushed the SWIFT-749/elg-provider branch from 46e46e8 to 8434e91 Compare April 28, 2020 04:51
@codecov-io
Copy link

codecov-io commented Apr 28, 2020

Codecov Report

Merging #446 into master will increase coverage by 0.12%.
The diff coverage is 90.44%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #446      +/-   ##
==========================================
+ Coverage   76.59%   76.72%   +0.12%     
==========================================
  Files         117      117              
  Lines       12984    13056      +72     
==========================================
+ Hits         9945    10017      +72     
  Misses       3039     3039              
Impacted Files Coverage Δ
Sources/MongoSwift/ConnectionPool.swift 59.47% <83.56%> (+12.25%) ⬆️
Sources/MongoSwift/MongoClient.swift 86.75% <93.75%> (+0.94%) ⬆️
Sources/MongoSwift/APM.swift 83.54% <100.00%> (+<0.01%) ⬆️
Sources/MongoSwift/Operations/Operation.swift 96.87% <100.00%> (+1.10%) ⬆️
Sources/MongoSwiftSync/MongoClient.swift 54.41% <100.00%> (ø)
Tests/MongoSwiftTests/AsyncTestUtils.swift 67.24% <100.00%> (+3.44%) ⬆️
Tests/MongoSwiftTests/MongoClientTests.swift 98.97% <100.00%> (+0.61%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 973acb2...7622137. Read the comment docs.

@kmahar
Copy link
Contributor Author

kmahar commented Apr 28, 2020

alright I've made significant changes based on our conversations, will make some comments inline. the gist is that the client now only tracks if it has been closed, which is just for the purpose of asserting in deinit. otherwise we let the connection pool and NIOThreadPool handle erroring.

Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some nits that are optional so lgtm 🚀

@kmahar kmahar merged commit b863152 into master May 1, 2020
@kmahar kmahar deleted the SWIFT-749/elg-provider branch May 1, 2020 00:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants