1.5.0/latest #13
1.5.0/latest #13
Conversation
|
Transmission of messages within:
|
There was a problem hiding this comment.
This library is not thread safe, the connection setup/disconnect should be protected with a lock. The Task also only supports a connection to one host at a time which should be documented. Although not too probable, someone might try to use it to connect to multiple separate RabbitMQs concurrently.
|
|
||
| return true; | ||
| // Commit under transaction when all of the messages have been received for the producer. | ||
| if (channel.MessageCount(inputParams.QueueName) == inputParams.WriteMessageCount) |
There was a problem hiding this comment.
Using message counts for specifying when to commit a message seems a bit error prone. It would be better to have a separate Task for committing the batch.
| CreateBasicPublishBatch(inputParams.ProcessExecutionId, channel).Publish(); | ||
| channel.TxCommit(); | ||
|
|
||
| if (channel.MessageCount(inputParams.QueueName) > 0) |
There was a problem hiding this comment.
Why rollback here, isn't the transaction committed right above this and rolling back here would have no effect or throw an error?
There was a problem hiding this comment.
if there are messages left in the bounded context all messages needs to be rollbacked.
| @@ -2,4 +2,5 @@ | |||
| <packages> | |||
| <package id="Microsoft.Diagnostics.Tracing.EventSource.Redist" version="1.1.28" targetFramework="net452" /> | |||
| <package id="RabbitMQ.Client" version="5.1.0" targetFramework="net452" /> | |||
| <package id="System.IO.Compression" version="4.3.0" targetFramework="net452" /> | |||
There was a problem hiding this comment.
This is no longer needed as the compression was removed?
| @@ -30,6 +30,8 @@ Tasks | |||
| | ExchangeName | string | Name of the exchange | sampleExchange | | |||
| | RoutingKey | string | Routing key (as in RabbitMQ specification) | sampleQueue | | |||
| | HostName | string | Address of the server hosting RabbitMQ | localhost or amqp://user:password@hostname:port/vhost | | |||
| | WriteMessageCount | string | Amount of messages in the buffer which will be sent over messaging queue as a batch. | 20 | | |||
| | ProcessExecutionId | string | Unique id of the process execution. | igbmdajlskdhlfjaeirjwkdjfasdflht | | |||
There was a problem hiding this comment.
This should probably instruct the user to use #process.executionid
| @@ -43,6 +45,7 @@ Tasks | |||
| | RoutingKey | string | Routing key (as in RabbitMQ specification) | sampleQueue | | |||
| | HostName | string | Address of the server hosting RabbitMQ | localhost or amqp://user:password@hostname:port/vhost | | |||
| | ConnectWithURI | bool | If true, hostname should be an URI | If false, use hostname only | | |||
| | ProcessExecutionId | string | Unique id of the process execution. | igbmdajlskdhlfjaeirjwkdjfasdflht | | |||
There was a problem hiding this comment.
This should probably instruct the user to use #process.executionid
| @@ -30,6 +30,8 @@ Tasks | |||
| | ExchangeName | string | Name of the exchange | sampleExchange | | |||
| | RoutingKey | string | Routing key (as in RabbitMQ specification) | sampleQueue | | |||
| | HostName | string | Address of the server hosting RabbitMQ | localhost or amqp://user:password@hostname:port/vhost | | |||
| | WriteMessageCount | string | Amount of messages in the buffer which will be sent over messaging queue as a batch. | 20 | | |||
There was a problem hiding this comment.
If we are going to use the WriteMessageCount to signal when the batch should be committed, that should be reflected in the documentation. IMHO it would be more clear to have the separate task for committing the batch instead. This way the functionality isn't "magic" for the user
Please review changes including