extract_server_update #2065
extract_server_update #2065
Conversation
server/client.go
Outdated
| @@ -3125,6 +3117,16 @@ func (c *client) deliverMsg(sub *subscription, subject, reply, mh, msg []byte, g | |||
| return true | |||
| } | |||
|
|
|||
| func (c *client) serveUpdate(client *Client, sub *subscription) { | |||
There was a problem hiding this comment.
I would rename this to make it a server receiver since when we call above, you already have srv. Also, maybe a rename to func (s *Server) updateRemoteSubscription(c *client, sub *subscription)?
server/client.go
Outdated
| func (c *client) serveUpdate(client *Client, sub *subscription) { | ||
| srv := client.srv | ||
|
|
||
| if srv.gateway.enabled { |
There was a problem hiding this comment.
I don't think ordering matters much, but trying to be consistent with most of the rest of the code (mainly in (*client).processUnsub(), I would change the order to do update route, then update gateway, then update leafnodes.
There was a problem hiding this comment.
I tried user the following sequence:
https://github.com/nats-io/nats-server/blob/master/server/client.go#L2972
It says that we need that sequence of execution. There we update gateway, route, and leafs.
By the way, if we have to have different sequences in different places, then the PR is not needed.
Should we have the order route, then update gateway, then update leafnodes. on both places?
There was a problem hiding this comment.
Like I said, the order may not matter much, but the comment was saying something but then doing something else. See, in non defer cases, we would call update route, gateway then leafnodes. But in the defer (which reverse the order), we have leafnodes, route and gateway. I am guessing that the "defer srv.updateLeafNodes(client.acc, sub, -1)" was added later, and not at the right position.
Like I said, I think ordering is not mandatory, but I like to try to be consistent. So in your new function, you should have the "route, gateway and leafnodes" in that that order please. Order in that function will always be the same, and the good thing is that the whole function is called as a defer, so we don't have to think about reordering list of defers..
alexpantyukhin
force-pushed
the
extract_server_update
branch
2 times, most recently
from
93e45b6 to
c0f50a3
Compare
|
@kozlovic All mentions are fixed. Ready for review. |
server/server.go
Outdated
| @@ -3356,3 +3356,12 @@ func (s *Server) setFirstPingTimer(c *client) { | |||
| d += time.Duration(addDelay) | |||
| c.ping.tmr = time.AfterFunc(d, c.processPingTimer) | |||
| } | |||
|
|
|||
| func (s *Server) updateRemoteSubscription(c *client, sub *subscription) { | |||
There was a problem hiding this comment.
Sorry for mentioning that now, but since we are just removing here, the name of the function should reflect that, or, maybe better, keep this name but add a delta int as the last parameter of this function (and pass delta in updateRouteSubscriptionMap(), gatewayUpdateSubInterest() and updateLeafNodes().
In the places you called updateRemoteSubscription() in this PR you would pass -1. There are only 1 or 2 places in the rest of the server code where we could possibly call this function with a positive delta, because in some cases we do some of the things based on the client's kind (CLIENT, etc..).
We internally need to have a deeper look at that, but I think having this function with a delta could make it be reusable later when we cleanup the rest of the code.
There was a problem hiding this comment.
Good input! I will fix asap
There was a problem hiding this comment.
@kozlovic fixed.
alexpantyukhin
force-pushed
the
extract_server_update
branch
from
c0f50a3 to
e16bebb
Compare
Resolves #NNNgit pull --rebase origin master)Resolves #
Changes proposed in this pull request:
/cc @nats-io/core