NoEcho Option Support #375
Conversation
Signed-off-by: Derek Collison <derek@nats.io>
Signed-off-by: Derek Collison <derek@nats.io>
Signed-off-by: Derek Collison <derek@nats.io>
| b, err := json.Marshal(cinfo) | ||
| if err != nil { | ||
| return _EMPTY_, ErrJsonParse | ||
| } | ||
|
|
||
| // Check if NoEcho is set and we have a server that supports it. |
There was a problem hiding this comment.
Why not do this test in processExpectedInfo() instead?
There was a problem hiding this comment.
I could if you think that is a better place. When forming the connectProto we know we have received the server info and are processing the options to some degree, hence why I stuck it there.
There was a problem hiding this comment.
That's fine I guess, although you could do this check at top of function, since there is no point in marshalling etc.. if we are going to return an error.
There was a problem hiding this comment.
I wanted a marshal error to take precedence.
| t.Fatalf("Error on publish: %v", err) | ||
| } | ||
| nc.Flush() | ||
| nc.Flush() |
There was a problem hiding this comment.
Also, especially in Travis, this is not enough to guarantee that there is no bug and message is not going to be incorrectly delivered. Maybe you would need 1 flush, but using a different connection, publish an other message, that message should be received, but not the first one.
All that being said, I do believe that the feature is tested in the server. We should focus here in testing APIs related features, which is make sure that the "echo" field is set as expected in the INFO, etc...
There was a problem hiding this comment.
Yes the two was intentional instead of having to do a sleep or gosched() call since first may have the scheduler unblock the flush call before firing the async callback. I could make it sync maybe.. Or just comment as to why there are two.
|
LGTM |
Signed-off-by: Derek Collison <derek@nats.io>
Added support to pass in a NoEcho option. Old servers that do not support will error.