-
Notifications
You must be signed in to change notification settings - Fork 188
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
Remove paused requests from LB #694
Conversation
@@ -2,6 +2,7 @@ | |||
|
|||
import java.util.List; | |||
|
|||
import com.google.common.base.Optional; |
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.
unnecessary import
return delete(getLBCleanupPath(requestId)); | ||
} | ||
|
||
public Optional<SingularityLoadBalancerUpdate> getLoadBalancerState(String requestId) { |
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.
what happens if getLoadBalancer()
state is called after createLBCleanupRequest()
, but no saveLoadBalancerState()
?
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.
From what I interpreted it would return an Optional.absent, which would tell the cleanup method that no request has been sent yet and the cleanup still needs to be started
LGTM but I'd like to get @wsorenson's 2 cents on it too |
Are there tests? |
Not yet, I can write some up today though |
public SingularityLoadBalancerUpdate delete(LoadBalancerRequestId loadBalancerRequestId, SingularityRequest request, SingularityDeploy deploy) { | ||
final List<String> serviceOwners = request.getOwners().or(Collections.<String> emptyList()); | ||
final Set<String> loadBalancerGroups = deploy.getLoadBalancerGroups().or(Collections.<String> emptySet()); | ||
final BaragonService lbService = new BaragonService(request.getId(), serviceOwners, deploy.getServiceBasePath().get(), loadBalancerGroups, deploy.getLoadBalancerOptions().orNull()); |
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.
Does the service not take optionals ? Seems awkward to have to transition between Optionals and empty collections here.
What is the difference between deleting request vs deleting tasks in Baragon? On unpause, will task additions automatically recreate what's been deleted? (I see no corresponding call) |
A |
So do we issue a DELETE on request removal? |
Yes, |
Added a test for the request lb cleanup queue and one small tweak for processing the cleanup |
Now that Baragon supports the
DELETE
request action for a few releases, this introduces lb cleanup for paused requests. Since our current LB cleanup queue is very task based, it would be difficult to accurately determine from a task's point of view that a request could be deleted form the LB. So, I implemented a request-based lb cleanup queue to go alongside our task-based one.@tpetr