-
Notifications
You must be signed in to change notification settings - Fork 37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Keep track of changes to the upstreams for a service within a batch on the Agent. #309
Conversation
257aefe
to
c8b670a
Compare
Optional<BaragonService> oldService; | ||
|
||
if (maybeRequest.isPresent()) { | ||
oldService = getOldService(maybeRequest.get()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we also save the old service instead of having to look it up again in processRequest?
serviceId = request.getLoadBalancerService().getServiceId(); | ||
existingUpstreamsForThisService = existingUpstreams.get(serviceId); | ||
if (existingUpstreamsForThisService == null || existingUpstreamsForThisService.isEmpty()) { | ||
existingUpstreamsForThisService = new ArrayList<>(stateDatastore.getUpstreams(serviceId)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are we resetting from the state datastore here? If it's empty wouldn't we assume that it's because we already looked it up and it was empty earlier?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is mostly to cover the case where we had an exception grabbing the upstreams the first time, for whatever reason: https://github.com/HubSpot/Baragon/pull/309/files/c8b670ab071ebc5d957307fdb5e8373560e21452#diff-2829d66da65c086b8c7d4fbb094196d3R100
Although I suppose we could just throw a RuntimeException there instead, which is what we do elsewhere in the code when try/catching around stateDatastore.getUpstreams()
.
No description provided.