-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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
HDDS-1217. Refactor ChillMode rules and chillmode manager. #558
Conversation
💔 -1 overall
This message was automatically generated. |
ab95869
to
4e43b44
Compare
Except protected checkstyle warning, remaining all have been addressed. |
💔 -1 overall
This message was automatically generated. |
* | ||
* @param <T> | ||
*/ | ||
public interface ChillModeExitRule<T> { |
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.
Just food for thought: You could actually still keep this as an interface and provide a default implementation for onMessage. New Java 8 contract change that avoids introducing inheritance purely for a default impl.
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.
Yes, it can be used but we don't want subclasses to implement this. So in the next patch, I made this method as final in abstract class. This came up when discussing with Nanda, so left this as an abstract class.
But your suggestion can be done, if we don't have such a requirement.
chillModeCutoff = conf.getDouble( | ||
HddsConfigKeys.HDDS_SCM_CHILLMODE_THRESHOLD_PCT, | ||
HddsConfigKeys.HDDS_SCM_CHILLMODE_THRESHOLD_PCT_DEFAULT); | ||
chillModeManager = manager; | ||
|
||
if (chillModeCutoff > 1.0 || chillModeCutoff < 0.0) { |
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.
FFT: Guava preconditions might improve readability here.
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
4e43b44
to
257fc60
Compare
🎊 +1 overall
This message was automatically generated. |
257fc60
to
ed2e04c
Compare
💔 -1 overall
This message was automatically generated. |
ed2e04c
to
ee72b89
Compare
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
* returns false. | ||
* @return boolean | ||
*/ | ||
public abstract boolean validate(); |
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 method can be made protected and all the implementation can also be protected.
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
* Actual processing logic for this rule. | ||
* @param report | ||
*/ | ||
public abstract void process(T report); |
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 can also be protected.
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
/** | ||
* Cleanup action's need to be done, once this rule is satisfied. | ||
*/ | ||
public abstract void cleanup(); |
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 can also be protected.
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
...op-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/chillmode/ChillModeExitRule.java
Show resolved
Hide resolved
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.
ChillModeExitRule
We can mandate the event handler adding logic of each and every ChillModeExitRule implementation by introducing an abstract abstract TypedEvent<T> getEventType()
method in ChillModeExitRule
class. Then we can make a call to this method from ChillModeExitRule
constructor. This will mandate each and every rule to provide it's EventType and we can add its event handler in ChillModeExitRule
.
public ChillModeExitRule(SCMChillModeManager chillModeManager,
String ruleName, EventQueue eventQueue) {
this.chillModeManager = chillModeManager;
this.ruleName = ruleName;
eventQueue.addHandler(getEventType(), this);
}
26b86a8
to
2f76f2b
Compare
Thank You @nandakumar131 for the review. |
Done |
💔 -1 overall
This message was automatically generated. |
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.
+1
Author: Aditya Toomula <atoomula@linkedin.com> Reviewers: Srini P<spunuru@linkedin.com> Closes apache#558 from atoomula/sql1
No description provided.