-
Notifications
You must be signed in to change notification settings - Fork 9.2k
HDDS-1193. Refactor ContainerChillModeRule and DatanodeChillMode rule. #534
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
Conversation
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.
when maxContainer is 0 then (containerWithMinReplicas.get() / maxContainer) will throw exception.
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.
In validate(), we call getCurrentContainerThreshold(),
public double getCurrentContainerThreshold() {
if (maxContainer == 0) {
return 1;
}
return (containerWithMinReplicas.doubleValue() / maxContainer);
}
public boolean validate() {
return getCurrentContainerThreshold() >= chillModeCutoff;
}
So, if maxContainer=0, in validate() call, we get 1 from getCurrentContainerThreshold() and do >= check.
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.
Before validation we should add new report data to threshold.
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 check is added, suppose this rule is already satisfied, I don't need to process it, so that is the reason for the first check.
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.
Updated the code with slight change.
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.
We should process data from this report before validating 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.
Done
c6f37a8 to
731eb26
Compare
|
Thank You @ajayydv for offline discussion. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
arp7
left a comment
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.
The patch looks good to me. I assume we don't need new unit tests since this is just refactoring and the existing unit tests are sufficient?
@ajayydv are you okay with the latest patch?
|
LGTM. +1 |
Samza currently depends on httpclient 4.5.2 and httpcore 4.4.5. However, httpclient 4.5.2 also has a direct dependency on httpcore 4.4.4, which is not backwards compatible with httpcore 4.4.5 since some classes were removed (e.g. ThreadSafe/NotThreadSafe annotation classes). Although this does not currently cause any direct build problems, there may be cases where this conflict introduces transitive dependency conflicts. In addition, this inconsistency can cause confusion in future development if those libraries need to be used. Author: Cameron Lee <[email protected]> Reviewers: Jagadish Venkatraman <[email protected]>, Shanthoosh Venkatraman <[email protected]> Closes apache#534 from cameronlee314/httpcore
No description provided.