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.

Sign up for GitHub

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

Jump to bottom

Add used memory amount to CircuitBreakingException message (#22521) #22693

Merged
merged 1 commit into from
Jan 30, 2017
Merged

Add used memory amount to CircuitBreakingException message (#22521) #22693

merged 1 commit into from
Jan 30, 2017

Conversation

bumbu
Copy link
Contributor

@bumbu bumbu commented Jan 19, 2017

Closes #22521

Tests don't check for the amount of used memory because that will tie-up the tests to the implementation.
From what I saw there are no tests to check that the used memory is calculated correctly, but it may be just my lack of knowledge of the codebase. Also it is out of the scope of original ticket.

@dakrone
Copy link
Member

dakrone commented Jan 19, 2017

@bumbu thanks for this! This looks pretty good, could you add a unit test to MemoryCircuitBreakerTests with exact numbers so we can assert the message looks correct? (I understand you can't easily add it to the integration tests since the desired memory is not constant)

@dakrone dakrone self-assigned this Jan 19, 2017
@dakrone dakrone self-requested a review January 19, 2017 16:34
@dakrone dakrone added :Core/Infra/Circuit Breakers Track estimates of memory consumption to prevent overload v5.3.0 v6.0.0-alpha1 labels Jan 19, 2017
@bumbu
Copy link
Contributor Author

bumbu commented Jan 20, 2017 •
edited
Loading

Hi @dakrone,

I added an assertion to MemoryCircuitBreakerTests to check for the numbers returned in the message. I didn't hard-code the number, instead I computed it right before the assertion. Didn't want to hard-code the number as how it is currently done gives more insight on what that number is.

Also I tested only for the first part of the number value (aka testing only for 1024 from [1024/1k]) as I didn't want to add one more dependency for computing the byte size value. The assertion should cover the message pretty well as the same number is used for both formatted and not formatted value. But I can easily add that if it will bring more value.

I squashed the commit.

dakrone
dakrone requested changes Jan 23, 2017
View reviewed changes
Copy link
Member

@dakrone dakrone left a comment

Choose a reason for hiding this comment

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

It looks like there's a compilation error, @bumbu otherwise this looks good, can you fix the error and then I'll merge this?

core/src/test/java/org/elasticsearch/common/breaker/MemoryCircuitBreakerTests.java
@@ -235,6 +235,9 @@ public void testConstantFactor() throws Exception {
fail("should never reach this");
} catch (CircuitBreakingException cbe) {
assertThat("breaker was tripped exactly twice", breaker.getTrippedCount(), equalTo(2L));

long newUsed = (long)(breaker.getUsed() * breaker.getOverhead());
assertThat(cbe.getMessage().contains("would be [" + newUsed "/"), equalTo(true));
Copy link
Member

Choose a reason for hiding this comment

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

This is missing a + between newUsed and "/", so it's not compiling currently

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad. I was sure that it's not possible to introduce bugs with such a small change... Lesson learned.

@bumbu
Copy link
Contributor Author

bumbu commented Jan 28, 2017

Hi @dakrone.
Sorry for the delay, I was on holidays.
I fixed the bug, run tests, squashed commits and rebased from master.
Please check.

@dakrone
Copy link
Member

dakrone commented Jan 30, 2017

@elasticmachine test this please

@dakrone dakrone merged commit 41abf6e into elastic:master Jan 30, 2017
@dakrone
Copy link
Member

dakrone commented Jan 30, 2017

Thanks @bumbu, I've merged this!

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

@dakrone dakrone dakrone requested changes

Assignees

@dakrone dakrone

Labels
:Core/Infra/Circuit Breakers Track estimates of memory consumption to prevent overload >enhancement v5.3.0 v6.0.0-alpha1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bumbu @dakrone @clintongormley