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

Fix calculation of available job capacity returned in HealthIndicator #503

Merged
merged 1 commit into from
May 15, 2017

Conversation

mprimi
Copy link
Contributor

@mprimi mprimi commented May 11, 2017

No description provided.

@mprimi mprimi requested a review from tgianos May 11, 2017 20:51
@mprimi mprimi self-assigned this May 11, 2017
@mprimi mprimi added the bug label May 11, 2017
@mprimi mprimi added this to the 3.1.0 milestone May 11, 2017
@tgianos tgianos modified the milestones: 3.0.6, 3.1.0 May 11, 2017
@tgianos
Copy link
Contributor

tgianos commented May 11, 2017

Please pull this against 3.0.x branch to target 3.0.6 release and then cherry pick into master after that.

@mprimi
Copy link
Contributor Author

mprimi commented May 11, 2017

Closing for the moment.

@mprimi mprimi closed this May 11, 2017
@mprimi mprimi modified the milestones: 3.1.0, 3.0.6 May 12, 2017
@mprimi mprimi reopened this May 15, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 88.804% when pulling 06d6792 on mprimi:master into 3f93a8c on Netflix:master.

@@ -84,8 +84,8 @@ public Health health() {
.withDetail(NUMBER_RUNNING_JOBS_KEY, this.jobMetricsService.getNumActiveJobs())
.withDetail(USED_MEMORY_KEY, usedMemory)
.withDetail(AVAILABLE_MEMORY, availableMemory)
.withDetail(AVAILABLE_DEFAULT_JOB_CAPACITY, availableMemory % defaultJobMemory)
.withDetail(AVAILABLE_MAX_JOB_CAPACITY, availableMemory % maxJobMemory)
.withDetail(AVAILABLE_DEFAULT_JOB_CAPACITY, availableMemory / defaultJobMemory)
Copy link
Contributor

Choose a reason for hiding this comment

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

Decided not to check divide by zero here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Argh, cherry-picked the wrong version of this commit somehow..
Don't merge please.

tgianos
tgianos previously approved these changes May 15, 2017
@mprimi
Copy link
Contributor Author

mprimi commented May 15, 2017

I just reopened this PR which still had the original change i made on master (i.e., no div by zero).
I hadn't actually cherry-picked the 3.0.x version and pushed.
Looks good now.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 88.706% when pulling c197935 on mprimi:master into 222c7a4 on Netflix:master.

@mprimi mprimi merged commit 2084fd1 into Netflix:master May 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants