Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow multiple channels to be created for a single connection #187

Merged
merged 2 commits into from
Apr 25, 2019
Merged

Allow multiple channels to be created for a single connection #187

merged 2 commits into from
Apr 25, 2019

Conversation

DougC
Copy link
Contributor

@DougC DougC commented Apr 25, 2019

[https://github.com//issues/186]

The aim here is to allow a client to create a connection out of a ConnectionStream and then use that to create multiple channels that different parts of the system can use. This supports the model recommended for rabbitmq where different threads operate on separate channels.

  • Added a createConnection method to the Connection[F[_]] trait alongside the existing createConnectionChannel
  • Created a Channeller trait (named to avoid clash with rabbit Channel class, up for a better name) with a createChannel method
  • Created a ChannelStream implementation that will create a stream of channels out of a provided connection
  • Created an AMQPConnection model to wrap a rabbit connection, in line with the AMQPChannel model
  • Bumped version to 1.2.1-SNAPSHOT for the moment

[#186]

The aim here is to allow a client to create a connection out of a ConnectionStream and then use that to create multiple channels that different parts of the system can use. This supports the model recommended for rabbitmq where different threads operate on separate channels.

* Added a `createConnection` method to the `Connection[F[_]]` trait alongside the existing `createConnectionChannel`
* Created a `Channeller` trait (named to avoid clash with rabbit `Channel` class, up for a better name) with a `createChannel` method
* Created a `ChannelStream` implementation that will create a stream of channels out of a provided connection
* Created an `AMQPConnection` model to wrap a rabbit connection, in line with the `AMQPChannel` model
* Bumped version to 1.2.1-SNAPSHOT for the moment
@codecov-io
Copy link

codecov-io commented Apr 25, 2019

Codecov Report

Merging #187 into master will decrease coverage by 3.22%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #187      +/-   ##
=========================================
- Coverage   97.72%   94.5%   -3.23%     
=========================================
  Files          15      16       +1     
  Lines         176     182       +6     
  Branches        2       3       +1     
=========================================
  Hits          172     172              
- Misses          4      10       +6
Impacted Files Coverage Δ
...main/scala/com/github/gvolpe/fs2rabbit/model.scala 100% <ø> (ø) ⬆️
...b/gvolpe/fs2rabbit/interpreter/ChannelStream.scala 0% <0%> (ø)
...ithub/gvolpe/fs2rabbit/interpreter/Fs2Rabbit.scala 85.71% <0%> (-2.53%) ⬇️

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 6dd5255...08d5b48. Read the comment docs.

Copy link
Member

@gvolpe gvolpe left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!


import com.github.gvolpe.fs2rabbit.model.AMQPChannel

// TODO: Find a better name - I've called it this to avoid clash with the rabbit class called Channel
Copy link
Member

Choose a reason for hiding this comment

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

Naming is hard 😄

It's fine and not so important in this case since it's just an internal algebra so we can leave it as is. Please remove the TODO comment :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I've removed the TODO. I haven't done any unit tests specifically for this. Is that okay?

Copy link
Member

Choose a reason for hiding this comment

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

Tests are always nice to have but it can be done in a different PR.

What I'm mostly concerned about is on whether we might run into thread-safety issues when creating multiple channels from the same connection. It would be great to add some tests for that (all tests run against a dockerized RabbitMQ).

@gvolpe gvolpe merged commit 929c614 into profunktor:master Apr 25, 2019
@DougC DougC deleted the issue-186-multiple-channels-per-connection branch April 25, 2019 11:06
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.

None yet

3 participants