Skip to content
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

don't include cleaning tasks in instance count #1505

Merged
merged 7 commits into from May 1, 2017
Merged

Conversation

darcatron
Copy link
Contributor

Tasks that are cleaning should not be part of the instance count when checking for over provisioned requests

@ssalinas

c = new Counter();
map.put(key, c);
}
Counter c = map.computeIfAbsent(key, k -> new Counter());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are not using the key in order to determine the value here, should use putIfAbsent instead of computeIfAbsent. Same for the one below

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small gotcha for putIfAbsent: turns out putIfAbsent returns the old value (null) so computeIfAbsent is what we want here (returns existing or computed)
bitmoji

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 good find, didn't realize that one

@@ -133,6 +134,10 @@ public void save(SingularityHostState hostState) throws InterruptedException {
numTasks.incr(pendingTaskId.getRequestId());
}

for (SingularityTaskId cleaningTaskId : taskManager.getCleanupTaskIds()) {
numTasks.decr(cleaningTaskId.getRequestId());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still want to include cleaning when checking for under provisioned, this will exclude it for both under and over? Trying to make sure we avoid any false alerts if possible

Copy link
Contributor Author

@darcatron darcatron Apr 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe so. I'm imagining a request that should have 3 instances, but has 2 running and 1 cleaning. Since we're still counting pending tasks, if one task is pending, then the count is correct, otherwise, it's under provisioned. I think counting cleaning tasks may actually have made us miss potential under provisioned requests in the past

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 cool, let's give this a go in staging/qa then

@ssalinas ssalinas modified the milestone: 0.15.0 Apr 19, 2017
@ssalinas
Copy link
Member

👍 lets get this into stable

@ssalinas
Copy link
Member

Just verifying the merge conflicts got resolved correctly and will merge this one too 👍

@ssalinas ssalinas merged commit c8b352b into master May 1, 2017
@ssalinas ssalinas deleted the provisioned branch May 1, 2017 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants