search

0x4b53 / amqp-rpc

🐰 Framework to use RabbitMQ as RPC
MIT License
45 stars 8 forks source link

Client: Implement NotifyPublish and NotifyReturn #82

Closed akarl closed 4 years ago

akarl commented 4 years ago

Client will wait for confirmation from amqp-server before continuing. And in the case of requests where no response is wanted, not return until the request has been confirmed.

If a message can not be routed or (if immediate) is set not be routed immediately, the client will return an error.

Returns are always handled (as long as the immediate or mandatory flags are set).

Confirmations can be controlled by the Client#WithPublishSettings or Client#WithConfirmMode.

This commit also improves some error logging to give some more information about the request/response.

bombsimon commented 4 years ago

@akarl Will review asap but feel free to toggle of wsl in the config it you hate it. Or just fix the errors. :)

akarl commented 4 years ago

No it’s great! I’ll fix them.

Sent from my iPhone

On 12 Nov 2019, at 19:09, Simon Sawert notifications@github.com wrote:

 @akarl Will review asap but feel free to toggle of wsl in the config it you hate it. Or just fix the errors. :)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

akarl commented 4 years ago

Just to clarify some questions:

no-wait: This argument can be sent to different methods on a channel. This makes the amqp server not wait for the command to be executed (consumer to be created, etc). In our case we want to do the topology setup sequentially on startup. Having no-wait set to true makes it brittle.

internal: Internal exchanges may only be used internally in the amqp server. ~You can consume from them. So that isn't something we want to set.~ EDIT: You can not consume from them. So that isn't something we want to set.

no-local: If the no-local field is set the server will not send messages to the connection that published them. This would have no affect on our part since we use different connections.

exclusive: Exclusive queues may only be accessed by the current connection, and are deleted when that connection closes. But since we use different connections, that won't work.

bombsimon commented 4 years ago

Just to clarify some questions:

no-wait: This argument can be sent to different methods on a channel. This makes the amqp server not wait for the command to be executed (consumer to be created, etc). In our case we want to do the topology setup sequentially on startup. Having no-wait set to true makes it brittle.

internal: Internal exchanges may only be used internally in the amqp server. You can consume from them. So that isn't something we want to set.

no-local: If the no-local field is set the server will not send messages to the connection that published them. This would have no affect on our part since we use different connections.

exclusive: Exclusive queues may only be accessed by the current connection, and are deleted when that connection closes. But since we use different connections, that won't work.

Thanks for the clarification!

bombsimon commented 4 years ago

For the future it would be better if you don't force push and then squash when merging instead. It's easier to follow the progress and to see what's been updated. :)

akarl commented 4 years ago

For the future it would be better if you don't force push and then squash when merging instead. It's easier to follow the progress and to see what's been updated. :)

Ahh, yes. Sorry about that!

bombsimon commented 4 years ago

Nice change with the stringify methods. 👍

One last thing that I forgot to mention (even though it's already approved): do you mind updating the README with the new changes and ensure that you've not removed anything mentioned there? I guess it's not that much since most of it is internal but at least add the WithConfirm() as an example and show that it can be set. In my opinion this is so specific that I think it's worth mentioning as a good feature of the library!

akarl commented 4 years ago

Nice change with the stringify methods. +1

One last thing that I forgot to mention (even though it's already approved): do you mind updating the README with the new changes and ensure that you've not removed anything mentioned there? I guess it's not that much since most of it is internal but at least add the WithConfirm() as an example and show that it can be set. In my opinion this is so specific that I think it's worth mentioning as a good feature of the library!

Yep!

codeclimate[bot] commented 4 years ago

Code Climate has analyzed commit e4ff378f and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 90.2% (50% is the threshold).

This pull request will bring the total coverage in the repository to 88.4% (1.3% change).

View more on Code Climate.