Deleting state #1386

Merged
merged 23 commits into from Feb 10, 2017

Conversation

Projects
None yet
3 participants
@darcatron
Contributor

darcatron commented Dec 27, 2016

Creates a new request state DELETING to better know when a request is still going through cleanup. Request State begins as DELETING and is changed to DELETED once all tasks for the request are cleaned. Currently using USER_REQUESTED_DESTROY to determine if the task is a delete request.

@tpetr @ssalinas

@ssalinas ssalinas modified the milestone: 0.14.0 Jan 3, 2017

+ return save(request, RequestState.DELETING, historyType, timestamp, user, message);
+ }
+
+ public SingularityCreateResult deleted(SingularityRequest request, RequestHistoryType historyType, long timestamp, Optional<String> user, Optional<String> message) {

This comment has been minimized.

@ssalinas

ssalinas Jan 3, 2017

Member

maybe for these methods we name them more like an action. i.e. markDeleted or markDeleting, just to clarify what they are doing. Especially since we already have a deleteRequest method (which could maybe be renamed to something like startRequestDelete?)

@ssalinas

ssalinas Jan 3, 2017

Member

maybe for these methods we name them more like an action. i.e. markDeleted or markDeleting, just to clarify what they are doing. Especially since we already have a deleteRequest method (which could maybe be renamed to something like startRequestDelete?)

- Optional<SingularityRequestHistory> maybeHistory = requestHistoryHelper.getLastHistory(requestId);
- if (maybeHistory.isPresent() && maybeHistory.get().getRequest().isLoadBalanced() && configuration.isDeleteRemovedRequestsFromLoadBalancer()) {
- createLbCleanupRequest(requestId, matchingActiveTaskIds);
+ if (matchingActiveTaskIds.iterator().hasNext()) {

This comment has been minimized.

@ssalinas

ssalinas Jan 3, 2017

Member

Maybe Iterables.isEmpty(matchingActiveTaskIds) just for cleanliness/readability

@ssalinas

ssalinas Jan 3, 2017

Member

Maybe Iterables.isEmpty(matchingActiveTaskIds) just for cleanliness/readability

+
+ private void cleanupRequestIfNoRemainingTasks(SingularityTaskId taskId, Map<String, List<String>> requestIdToTaskIds) {
+ List<String> requestTasks = requestIdToTaskIds.get(taskId.getRequestId());
+ requestTasks.remove(taskId.getId());

This comment has been minimized.

@ssalinas

ssalinas Jan 3, 2017

Member

requestIdToTaskIds isn't guaranteed to have the key present, will want to check that or this line will cause NPEs

@ssalinas

ssalinas Jan 3, 2017

Member

requestIdToTaskIds isn't guaranteed to have the key present, will want to check that or this line will cause NPEs

This comment has been minimized.

@darcatron

darcatron Jan 9, 2017

Contributor

ah, I didn't notice we would hit this method for other cleanup types that weren't in the requestIdToTaskIds map. updated to return early if it's null
bitmoji

@darcatron

darcatron Jan 9, 2017

Contributor

ah, I didn't notice we would hit this method for other cleanup types that weren't in the requestIdToTaskIds map. updated to return early if it's null
bitmoji

@ssalinas

This comment has been minimized.

Show comment
Hide comment
@ssalinas

ssalinas Jan 3, 2017

Member

Overall looks good, the few comments I left and a test that needs to be fixed:

  • SingularityHistoryTest.testMessage - count of history items is wrong now that we are creating another history entry
Member

ssalinas commented Jan 3, 2017

Overall looks good, the few comments I left and a test that needs to be fixed:

  • SingularityHistoryTest.testMessage - count of history items is wrong now that we are creating another history entry

darcatron added some commits Jan 9, 2017

SingularityDeleteResult deleteResult = delete(getRequestPath(request.getId()));
+ markDeleting(request, RequestHistoryType.DELETED, System.currentTimeMillis(), user, message);
+
LOG.info("Request {} deleted ({}) by {} - {}", request.getId(), deleteResult, user, message);

This comment has been minimized.

@darcatron

darcatron Jan 9, 2017

Contributor

@ssalinas should we adjust these log messages too? They're a bit misleading since they kind of jump the gun on the status

@darcatron

darcatron Jan 9, 2017

Contributor

@ssalinas should we adjust these log messages too? They're a bit misleading since they kind of jump the gun on the status

This comment has been minimized.

@ssalinas

ssalinas Jan 9, 2017

Member

Yeah, might be good for the log line to suggest it was 'enqueued for delete' rather than 'deleted'

@ssalinas

ssalinas Jan 9, 2017

Member

Yeah, might be good for the log line to suggest it was 'enqueued for delete' rather than 'deleted'

@@ -328,9 +334,12 @@ public SingularityDeleteResult deleteRequest(SingularityRequest request, Optiona
saveHistory(new SingularityRequestHistory(now, user, RequestHistoryType.DELETED, request, message));
+ // moves RequestState to DELETED
SingularityDeleteResult deleteResult = delete(getRequestPath(request.getId()));

This comment has been minimized.

@ssalinas

ssalinas Jan 12, 2017

Member

If we want to still be able to grab request data while it is in DELETING state, we shouldn't have this line here. This deletes all of the current request data. We should probably move this step to be triggered by the cleaner after everything is done (i.e. when the active deploy cleanup is happening)

@ssalinas

ssalinas Jan 12, 2017

Member

If we want to still be able to grab request data while it is in DELETING state, we shouldn't have this line here. This deletes all of the current request data. We should probably move this step to be triggered by the cleaner after everything is done (i.e. when the active deploy cleanup is happening)

@@ -1,5 +1,5 @@
package com.hubspot.singularity;
public enum SingularityDeleteResult {
- DELETED, DIDNT_EXIST;
+ DELETING, DELETED, DIDNT_EXIST;

This comment has been minimized.

@darcatron

darcatron Jan 12, 2017

Contributor

added this state to better reflect the state of the delete when it's first requested

@darcatron

darcatron Jan 12, 2017

Contributor

added this state to better reflect the state of the delete when it's first requested

@@ -16,7 +16,7 @@
private final Optional<String> message;
public enum RequestHistoryType {
- CREATED, UPDATED, DELETED, PAUSED, UNPAUSED, ENTERED_COOLDOWN, EXITED_COOLDOWN, FINISHED, DEPLOYED_TO_UNPAUSE, BOUNCED, SCALED, SCALE_REVERTED;
+ CREATED, UPDATED, DELETING, DELETED, PAUSED, UNPAUSED, ENTERED_COOLDOWN, EXITED_COOLDOWN, FINISHED, DEPLOYED_TO_UNPAUSE, BOUNCED, SCALED, SCALE_REVERTED;

This comment has been minimized.

@darcatron

darcatron Jan 12, 2017

Contributor

added this state to better reflect the state of the request history

@darcatron

darcatron Jan 12, 2017

Contributor

added this state to better reflect the state of the request history

@@ -346,7 +345,7 @@ private void drainRequestCleanupQueue() {
}
break;
case DELETING:
- if (requestWithState.isPresent()) {
+ if (requestWithState.isPresent() && requestWithState.get().getState() != RequestState.DELETING) {

This comment has been minimized.

@darcatron

darcatron Jan 12, 2017

Contributor

added this to make sure deleting states aren't ignored and will be cleaned up properly

@darcatron

darcatron Jan 12, 2017

Contributor

added this to make sure deleting states aren't ignored and will be cleaned up properly

@ssalinas

This comment has been minimized.

Show comment
Hide comment
@ssalinas

ssalinas Jan 13, 2017

Member

mentioned in person, but to document here as well. Last change needed for this would be to remove the piece of ui code that makes the Remove button redirect you back to the requests page. Should be gtg after that

Member

ssalinas commented Jan 13, 2017

mentioned in person, but to document here as well. Last change needed for this would be to remove the piece of ui code that makes the Remove button redirect you back to the requests page. Should be gtg after that

@darcatron

This comment has been minimized.

Show comment
Hide comment
@darcatron

darcatron Jan 17, 2017

Contributor

@ssalinas cool to move this into staging tmrw morning?

Contributor

darcatron commented Jan 17, 2017

@ssalinas cool to move this into staging tmrw morning?

@ssalinas

This comment has been minimized.

Show comment
Hide comment
@ssalinas

ssalinas Jan 17, 2017

Member

@darcatron you can just do a then={fetchRequestAndHistory} rather that doing the window.reload in the ui portion. That will trigger a re-fetch of the request data from the api, but we won't need to reload the entire page. After that small change, yeah, think we are good for staging

Member

ssalinas commented Jan 17, 2017

@darcatron you can just do a then={fetchRequestAndHistory} rather that doing the window.reload in the ui portion. That will trigger a re-fetch of the request data from the api, but we won't need to reload the entire page. After that small change, yeah, think we are good for staging

@@ -458,6 +462,24 @@ private TaskCleanupType pause(SingularityRequestCleanup requestCleanup, Iterable
return cleanupType;
}
+ private void delete(SingularityRequestCleanup requestCleanup, Iterable<SingularityTaskId> activeTaskIds){

This comment has been minimized.

@darcatron

darcatron Jan 20, 2017

Contributor

@ssalinas I kept the name as delete since it followed the pattern set by the other actions (pause, bounce, etc.)

@darcatron

darcatron Jan 20, 2017

Contributor

@ssalinas I kept the name as delete since it followed the pattern set by the other actions (pause, bounce, etc.)

@darcatron

This comment has been minimized.

Show comment
Hide comment
@darcatron

darcatron Jan 20, 2017

Contributor

Made the fixes and tested them locally. The request tasks are deleted appropriately and the request is then re-enqueued for cleanup. The user and message are preserved as well.

Also, I noticed the deploy and request history weren't updating so I fixed that. This is especially helpful when deleting a request so a user can see that it goes to a deleting state and then deleted state without refreshing the page

@ssalinas

Contributor

darcatron commented Jan 20, 2017

Made the fixes and tested them locally. The request tasks are deleted appropriately and the request is then re-enqueued for cleanup. The user and message are preserved as well.

Also, I noticed the deploy and request history weren't updating so I fixed that. This is especially helpful when deleting a request so a user can see that it goes to a deleting state and then deleted state without refreshing the page

@ssalinas

@ssalinas

let's remove those output.log and the .json files from your testing and address the two comments. Should be good to re-test in staging after that.

@@ -91,7 +91,9 @@ const mapDispatchToProps = (dispatch, ownProps) => {
FetchRequest.trigger(ownProps.params.requestId, true),
FetchActiveTasksForRequest.trigger(ownProps.params.requestId),
FetchScheduledTasksForRequest.trigger(ownProps.params.requestId),
- FetchTaskCleanups.trigger()
+ FetchTaskCleanups.trigger(),
+ FetchDeploysForRequest.trigger(ownProps.params.requestId, 5, 1),

This comment has been minimized.

@ssalinas

ssalinas Jan 23, 2017

Member

We don't want to be fetching these two here. These are actions that will auto-refresh on an interval. Since deploy history and request history are paginated, fetching this would reset the user back to the first page on a schedule. We should instead use the then values of the buttons you are editing to make sure the correct api calls are made in reaction to a user action.

Additionally, the deploy history won't be changing in our request deletion updates, only the request history

@ssalinas

ssalinas Jan 23, 2017

Member

We don't want to be fetching these two here. These are actions that will auto-refresh on an interval. Since deploy history and request history are paginated, fetching this would reset the user back to the first page on a schedule. We should instead use the then values of the buttons you are editing to make sure the correct api calls are made in reaction to a user action.

Additionally, the deploy history won't be changing in our request deletion updates, only the request history

@@ -162,7 +162,7 @@ const RequestActionButtons = ({requestParent, fetchRequest, fetchRequestHistory,
const navigateAwayOnSuccess = (response) => {
if (response.statusCode === 200) {
- router.push('/requests');
+ then={fetchRequestAndHistory}

This comment has been minimized.

@ssalinas

ssalinas Jan 23, 2017

Member

can move this a few lines down and get rid of the navigateAwayOnSuccess method:
<RemoveButton requestId={request.id} then={fetchRequestAndHistoryAndActiveTasks}>

  • Don't need the 200 check here since we are staying on the page regardless of the outcome of the api call
  • Added the ...AndActiveTasks portion because they will likely be updated to cleaning after the deletion is requested
@ssalinas

ssalinas Jan 23, 2017

Member

can move this a few lines down and get rid of the navigateAwayOnSuccess method:
<RemoveButton requestId={request.id} then={fetchRequestAndHistoryAndActiveTasks}>

  • Don't need the 200 check here since we are staying on the page regardless of the outcome of the api call
  • Added the ...AndActiveTasks portion because they will likely be updated to cleaning after the deletion is requested
+ runBeforeKillId = Optional.of(shellRequest.getId());
+ }
+
+ taskManager.createTaskCleanup(new SingularityTaskCleanup(requestCleanup.getUser(), TaskCleanupType.USER_REQUESTED_DESTROY, start, taskId, requestCleanup.getMessage(), requestCleanup.getActionId(), runBeforeKillId));

This comment has been minimized.

@ssalinas

ssalinas Jan 24, 2017

Member

missed this one initially, my fault here. This has some unintended consequences. With this in place, any task that is forcibly killed by a user will then cause the request to be deleted as well. We need a new TaskCleanupType that is separate. Something like REQUEST_DELETING with killLongRunningTaskInstantly and killNonLongRunningTaskInstantly both set to true like they are for the destroy

de9a21d

@ssalinas

ssalinas Jan 24, 2017

Member

missed this one initially, my fault here. This has some unintended consequences. With this in place, any task that is forcibly killed by a user will then cause the request to be deleted as well. We need a new TaskCleanupType that is separate. Something like REQUEST_DELETING with killLongRunningTaskInstantly and killNonLongRunningTaskInstantly both set to true like they are for the destroy

de9a21d

@ssalinas ssalinas added the hs_stable label Feb 3, 2017

@darcatron

This comment has been minimized.

Show comment
Hide comment
@darcatron

darcatron Feb 8, 2017

Contributor

Added a test to make sure that a single deleted task doesn't delete the entire request. Going to go through the branch merging (staging, qa, stable) to get it updated

Contributor

darcatron commented Feb 8, 2017

Added a test to make sure that a single deleted task doesn't delete the entire request. Going to go through the branch merging (staging, qa, stable) to get it updated

@ssalinas

This comment has been minimized.

Show comment
Hide comment
@ssalinas

ssalinas Feb 10, 2017

Member

LGTM

Member

ssalinas commented Feb 10, 2017

LGTM

@darcatron darcatron merged commit cb7271f into master Feb 10, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@darcatron darcatron deleted the deleting-state branch Feb 10, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment