Route registry enhancements #478
Conversation
maxmoehl
changed the title
cf-foundation-community-automation
bot
moved this to Inbox
in Application Runtime Platform Working Group
maxmoehl
force-pushed
the
maxmoehl/route-registry-enhancements
branch
from
f58bd8f to
09dd5ac
Compare
|
We've validated this in one of our environments and the metrics now show up with the "refreshed" tag. As expected, the number of route messages labeled as "updated" went down a lot as those are now "refreshed". The logs also behave as expected with the refresh logs (which have a high volume) being not emitted if you set the log level to info. We also ran the routing-release test suite for gorouter and it passes. |
| } | ||
|
|
||
| func (r *RouteRegistry) unregister(uri route.Uri, endpoint *route.Endpoint) { | ||
| func (r *RouteRegistry) unregister(uri route.Uri, endpoint *route.Endpoint) (endpointRemoved, routeRemoved bool) { |
There was a problem hiding this comment.
Can we pleadse have proper types for endpointRemoved and routeRemoved, similar to the poolPutResult?
There was a problem hiding this comment.
Well these are just bools... I'd rather not introduce aliases for true and false 😄
There was a problem hiding this comment.
Why not, still better than having
return true, false
return false, false
return true, true
in the code, where you need to read function signatures to understand the meaning.
There was a problem hiding this comment.
This looks pretty clear to me, at least in IntelliJ/GoLand
There was a problem hiding this comment.
I agree with @hoffmaen. The coding would be more readable. But your call.
|
One more thing: We log the registration of new routes very deep in the code (in |
maxmoehl
force-pushed
the
maxmoehl/route-registry-enhancements
branch
from
dc96907 to
b49cabf
Compare
Resolves: cloudfoundry#468 Co-Authored-By: Clemens Hoffmann <[email protected]>
maxmoehl
force-pushed
the
maxmoehl/route-registry-enhancements
branch
from
b98740e to
1fea46d
Compare
src/code.cloudfoundry.org/gorouter/metrics/metricsreporter_test.go
Outdated
Show resolved
Hide resolved
src/code.cloudfoundry.org/gorouter/metrics/metricsreporter_test.go
Outdated
Show resolved
Hide resolved
| @@ -88,32 +88,42 @@ func (r *RouteRegistry) Register(uri route.Uri, endpoint *route.Endpoint) { | |||
| return | |||
| } | |||
|
|
|||
| endpointAdded := r.register(uri, endpoint) | |||
| poolPutResult, routeAdded := r.register(uri, endpoint) | |||
There was a problem hiding this comment.
Maybe routePoolAdded instead of routeAdded? It is clearer then.
Resolves: cloudfoundry#468 Co-Authored-By: Clemens Hoffmann <[email protected]>
Co-Authored-By: Clemens Hoffmann <[email protected]> Co-Authored-By: Soha Alboghdady <[email protected]>
Co-Authored-By: Clemens Hoffmann <[email protected]> Co-Authored-By: Soha Alboghdady <[email protected]>
maxmoehl
force-pushed
the
maxmoehl/route-registry-enhancements
branch
from
1fea46d to
0eb9044
Compare
maxmoehl
force-pushed
the
maxmoehl/route-registry-enhancements
branch
from
e1bf949 to
e648904
Compare
Summary
Rework some of the route registry logic to reduce nested ifs and align some of the logic.
Backward Compatibility
Breaking Change? No