Improved logs and message ack.

porthos-rpc / porthos-go

A RPC over AMQP library for the Go programming language

BSD 3-Clause "New" or "Revised" License

7 stars 4 forks source link

Improved logs and message ack. #10

Closed skrater closed 8 years ago

gfronza commented 8 years ago

@skrater just some thoughts (something to discuss):

why exposing the option autoAck? not sure if this should be a developer's choice.
what is log.Success for? I've never seen that before, looks more like usage of log.Debug.

skrater commented 8 years ago

@gfronza

I don't know what kind of microservice can be made, sometimes it is not necessary to ensure that the message was delivered (for performance reasons).
Yes, log.Success is just for Debug, like the others logs. You suggest change to log.Debug, or we don't need that? My intention with log.Success is indicate that the message was processed and delivered to client.

gfronza commented 8 years ago

@skrater Well, setting autoAck to false won't behave as you said. You are just telling (or not) to RabbitMQ to handle the ack operation instead of the client code. IMHO, we must have a much stronger reason to open this possibility to developers, because when something goes wrong, we'll have two possibilities to cover up.

As for the logger thing, log.Success is just a little bit unusual (outside of the well known [info, debug, warn, error] world). So I would change to log.Debug, but that's just my opinion. @alexandrevicenzi what do you think?

skrater commented 8 years ago

@gfronza AFAIK, autoAck just telling to ack the message when he is delivered to the server (porthos), not when he is processed. This will flush the queue much faster and improve performance, but it need to be a developer choice, he needs to know that some messages will be lost when a panic goes up. Thats not a cruticial feature, i just did not want to leave ackMessages hardcoded.

About the log, I think it would look better with Debug, i will change it.

Ps.: This discussion is being better for my English than for Porthos. kkkk

gfronza commented 8 years ago

I still believe that we should only have 1 option. autoAck seems to be more appropriate for rpc calls (where we have really aggressive timeouts thresholds) and we avoid then ack call.

I'll merge it, but we might consider having a default value for the autoAck option (true).