-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
HBASE-26872 Load rate calculator for cost functions should be more precise #4253
Conversation
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
Hi, @bbeaudreault , would you want to review this PR in your convenience? Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change makes sense! The code change itself looks good. I had 1 minor request for the test, if it makes sense
@@ -519,6 +517,31 @@ public void testRegionLoadCost() { | |||
assertEquals(2.5, result, 0.01); | |||
} | |||
|
|||
@Test | |||
public void testRegionLoadCostWhenDecrease() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correct me if im wrong, but i believe this is testing the case where region loads go 1, 2, 0, 4
. So it uses a 0 value in the 3rd slot. This works to verify your change, but I wonder if it would be more comprehensive to test 1, 2, 1, 4
. This was just be more indicative of a real reset where the value starts at 0 but then gets incremented again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, good idea, @bbeaudreault. It's reasonable to test the [1, 2, 1, 4], since it's rare to see 0 in the real scenarios, and I used 0 to describe the issue more clearly. Thanks.
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
…ecise (#4253) Signed-off-by: Bryan Beaudreault <bbeaudreault@hubspot.com> Signed-off-by: Viraj Jasani<virajjasani@apache.org>
…ecise (#4253) Signed-off-by: Bryan Beaudreault <bbeaudreault@hubspot.com> Signed-off-by: Viraj Jasani<virajjasani@apache.org>
…ecise (#4253) Signed-off-by: Bryan Beaudreault <bbeaudreault@hubspot.com> Signed-off-by: Viraj Jasani<virajjasani@apache.org>
…ecise (#4253) Signed-off-by: Bryan Beaudreault <bbeaudreault@hubspot.com> Signed-off-by: Viraj Jasani<virajjasani@apache.org>
No description provided.