Skip processing of INFO.ConnectURLs when the array is empty. #352
Conversation
Continuation of #344. We don't want to remove discovered servers from the pool if we get an INFO with empty array. The new servers will send arrays with at least their own URL, but older servers or in some situations, the array could be empty (omitted) and we should not treat this as if there were no server at all in the cluster.
| return err | ||
| } | ||
| // Copy content into connection's info structure. | ||
| nc.info = ncInfo |
There was a problem hiding this comment.
Should we replace what was there if the array is empty and we ignore it?
There was a problem hiding this comment.
I did that first, but there is no point since the array is actually only used in this function.
There was a problem hiding this comment.
Then why make a copy and not just de-marshal direct into nc.info like it was?
There was a problem hiding this comment.
That's what I was doing before, but then I realized that we should always clear before the unmarshal because otherwise, if a field of the structure is set before calling unmarshal, but is empty in the incoming protocol, then it would not be set to empty in unmarshal. In other words, for instance if ConnectURLs is not empty before the call to Unmarshal, but the incoming protocol has an empty array, after the call it would seem as if ConnectURLs was set. In that specific case it would be fine, but as a general practice I think it is bad since we may not really use what comes from the protocol if we "reuse" the nc.Info for the Unmarshal call.
|
LGTM |
Continuation of #344. We don't want to remove discovered servers
from the pool if we get an INFO with empty array. The new servers
will send arrays with at least their own URL, but older servers
or in some situations, the array could be empty (omitted) and we
should not treat this as if there were no server at all in the
cluster.