Amqp: reusable connections #34
Conversation
bobanco
commented
Apr 11, 2018
bobanco
commented
- Add AMQP cached connection
- Add tests
|
|
||
| [Fact] | ||
| public void | ||
| The_AMQP_default_connection_providers_should_create_new_a_new_connection_per_invocation_of_LocalAmqpConnection() |
There was a problem hiding this comment.
"new_a_new" typo.
| { | ||
|
|
||
| } | ||
| public static AmqpLocalConnectionProvider Instance=> new AmqpLocalConnectionProvider(); |
There was a problem hiding this comment.
Is it necessary to create a new instance on every property invocation? It seems it can be cached in a static field.
There was a problem hiding this comment.
I think we could omit the property itself and use a static field, what do you think?
|
|
||
| } | ||
| public static AmqpLocalConnectionProvider Instance=> new AmqpLocalConnectionProvider(); | ||
| public override IConnection Get() => new ConnectionFactory().CreateConnection(); |
There was a problem hiding this comment.
Same as above, could ConnectionFactory be cached?
| { | ||
| public class AmqpConnectionProvidersTest : Akka.TestKit.Xunit2.TestKit | ||
| { | ||
| private readonly ActorMaterializer _mat; |
There was a problem hiding this comment.
_mat is not used anywhere.
| new AmqpDetailsConnectionProvider(new List<(string host, int port)> { (host, port) }); | ||
|
|
||
| public AmqpDetailsConnectionProvider WithHostsAndPorts((string host, int port) hostAndPort, | ||
| params (string host, int port)[] hostAndPortList) |
There was a problem hiding this comment.
Why such a strange signature? Would the following work?
public AmqpDetailsConnectionProvider WithHostsAndPorts(params (string host, int port)[] hostAndPortList)| public override string ToString() | ||
| { | ||
| return | ||
| $"AmqpDetailsConnectionProvider(HostAndPortList=({HostAndPortList.Select(x => $"[{x.host}:{x.port}]").Aggregate((left, right) => $"{right}, {left}")}), Credentials={Credentials}, VirtualHost={VirtualHost})"; |
There was a problem hiding this comment.
It's a bit hard to see what Aggregate does here. What about more common string.Join?
$"AmqpDetailsConnectionProvider(HostAndPortList=({string.Join(", ", HostAndPortList.Select(x => $"[{x.host}:{x.port}]"))}), Credentials={Credentials}, VirtualHost={VirtualHost})";|
|
||
| public IReadOnlyList<(string host, int port)> HostAndPortList => _hostAndPortList.Any() | ||
| ? _hostAndPortList.ToList() | ||
| : new List<(string host, int port)> {(_factory.HostName, _factory.Port)}; |
There was a problem hiding this comment.
I can be simplified:
public IReadOnlyList<(string host, int port)> HostAndPortList => _hostAndPortList.Any()
? _hostAndPortList
: new [] {(_factory.HostName, _factory.Port)};| params (string host, int port)[] hostAndPortList) | ||
| { | ||
| return new AmqpConnectionFactoryConnectionProvider(_factory, | ||
| new List<(string host, int port)>(hostAndPortList.ToList()) {hostAndPort}); |
|
|
||
| public sealed class AmqpCachedConnectionProvider : AmqpConnectionProvider | ||
| { | ||
| private AtomicReference<IState> _state = new AtomicReference<IState>(Empty.Instance); |
There was a problem hiding this comment.
Can be made readonly.
| } | ||
| public override IConnection Get() | ||
| { | ||
| var factory = new ConnectionFactory(); |
There was a problem hiding this comment.
Are you sure it's ok to create a ConnectionFactory every time Get is called?
|
@bobanco still working on this? |
|
@Arkatufus is this PR still necessary? |
|
It looks promising, but the original fork is gone. |
