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

Introduce an incremental bounce #767

Merged
merged 13 commits into from Dec 10, 2015

Conversation

Projects
None yet
2 participants
@ssalinas
Member

ssalinas commented Nov 19, 2015

Currently, in order to bounce a request which has SEPARATE* placement, you need to be running instance_count * 2 slaves. This can be quite a pain for requests with many instances. This PR creates a new type of bounce where we shut down old tasks as new ones are spun up (vs waiting for all new tasks to be ready first). Using this, you only need a minimum of instance_count + 1 slaves to bounce the same request

@ssalinas ssalinas changed the title from (WIP) introduce an incremental bounce to Introduce an incremental bounce Nov 24, 2015

@ssalinas

This comment has been minimized.

Show comment
Hide comment
@ssalinas

ssalinas Nov 24, 2015

Member

Got this working well in staging, also added an option for it in the UI
screen shot 2015-11-24 at 4 48 01 pm

Member

ssalinas commented Nov 24, 2015

Got this working well in staging, also added an option for it in the UI
screen shot 2015-11-24 at 4 48 01 pm

@ssalinas ssalinas added the hs_staging label Nov 24, 2015

Show outdated Hide outdated .../src/main/java/com/hubspot/singularity/scheduler/SingularityCleaner.java
@@ -362,9 +386,19 @@ private void drainTaskCleanupQueue() {
return;
}
Map<String, Integer> incrementalBounceRemainingInstanceMap = new HashMap<>();

This comment has been minimized.

@tpetr

tpetr Nov 25, 2015

Member

can you clarify what this map does? it tracks how many tasks still need to bounce per request?

@tpetr

tpetr Nov 25, 2015

Member

can you clarify what this map does? it tracks how many tasks still need to bounce per request?

This comment has been minimized.

@ssalinas

ssalinas Nov 25, 2015

Member

When doing our cleanup, each task doesn't know what has happened to the task before it. This way we can keep track of how many cleaning tasks were already killed in this cleanup run. (i.e. if we have room to kill one cleaning task in our incremental bounce, now we'll know when we get to the second that we've already done that and we shouldn't kill the second)

@ssalinas

ssalinas Nov 25, 2015

Member

When doing our cleanup, each task doesn't know what has happened to the task before it. This way we can keep track of how many cleaning tasks were already killed in this cleanup run. (i.e. if we have room to kill one cleaning task in our incremental bounce, now we'll know when we get to the second that we've already done that and we shouldn't kill the second)

tpetr added some commits Dec 4, 2015

@ssalinas ssalinas added the hs_qa label Dec 7, 2015

@tpetr

This comment has been minimized.

Show comment
Hide comment
@tpetr

tpetr Dec 8, 2015

Member

does this need to be a mutable collection? are we going to be modifying it?

does this need to be a mutable collection? are we going to be modifying it?

This comment has been minimized.

Show comment
Hide comment
@tpetr

tpetr Dec 8, 2015

Member

also, just a reminder to myself: getKilledTaskIdRecords() == list of tasks that we have issued kills to -- these records get cleaned up when we receive a TASK_KILLED status update from the executor

Member

tpetr replied Dec 8, 2015

also, just a reminder to myself: getKilledTaskIdRecords() == list of tasks that we have issued kills to -- these records get cleaned up when we receive a TASK_KILLED status update from the executor

This comment has been minimized.

Show comment
Hide comment
@tpetr

tpetr Dec 8, 2015

Member

last thing -- is it worth just caching the SingularityTaskId here, instead of re-generating the list each time it's called?

Member

tpetr replied Dec 8, 2015

last thing -- is it worth just caching the SingularityTaskId here, instead of re-generating the list each time it's called?

This comment has been minimized.

Show comment
Hide comment
@ssalinas

ssalinas Dec 8, 2015

Member

Yeah probably worth doing it that way. I'll update that and the immutable piece

Member

ssalinas replied Dec 8, 2015

Yeah probably worth doing it that way. I'll update that and the immutable piece

@tpetr

This comment has been minimized.

Show comment
Hide comment
@tpetr

tpetr Dec 8, 2015

Member

won't this taint the getCleaningTasks() cache collection, since it's mutable?

won't this taint the getCleaningTasks() cache collection, since it's mutable?

This comment has been minimized.

Show comment
Hide comment
@ssalinas

ssalinas Dec 8, 2015

Member

Updated for this and not rebuilding the task id list each time in 48e7210

Member

ssalinas replied Dec 8, 2015

Updated for this and not rebuilding the task id list each time in 48e7210

@tpetr

This comment has been minimized.

Show comment
Hide comment
@tpetr

tpetr Dec 9, 2015

Member

LGTM

Member

tpetr commented Dec 9, 2015

LGTM

@tpetr tpetr added this to the 0.4.6 milestone Dec 9, 2015

@ssalinas ssalinas added the hs_stable label Dec 9, 2015

ssalinas added a commit that referenced this pull request Dec 10, 2015

@ssalinas ssalinas merged commit bfbeec3 into master Dec 10, 2015

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

@ssalinas ssalinas deleted the incremental_bounce branch Dec 10, 2015

@ssalinas ssalinas referenced this pull request Dec 14, 2015

Merged

Bounce tests #807

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