-
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-23083 Collect Executor status info periodically and report to metric system #664
Conversation
6bbf89a
to
5ad507e
Compare
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
5ad507e
to
8e37736
Compare
🎊 +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.
A few nits. Looks good so far.
@@ -1191,6 +1191,9 @@ | |||
"hbase.node.health.failure.threshold"; | |||
public static final int DEFAULT_HEALTH_FAILURE_THRESHOLD = 3; | |||
|
|||
public static final String EXECUTOR_STATUS_COLLECTE_ENABLED = | |||
"hbase.executors.status.collect.enabled"; | |||
public static final boolean DEFAULT_EXECUTOR_STATUS_COLLECTE_ENABLED = true; |
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.
COLLECTE<= extra 'E'.
public ExecutorStatusChore(int sleepTime, Stoppable stopper, ExecutorService service, | ||
MetricsRegionServerSource metrics) { | ||
super("ExecutorStatusChore", stopper, sleepTime); | ||
LOG.info("ExecutorStatusChore runs every " + StringUtils.formatTime(sleepTime)); |
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.
s/runs every/runs every {}/ and s/+/,/... i.e. use parameterized logging.
hbase-server/src/main/java/org/apache/hadoop/hbase/ExecutorStatusChore.java
Show resolved
Hide resolved
HConstants.DEFAULT_EXECUTOR_STATUS_COLLECTE_ENABLED)) { | ||
int sleepTime = this.conf.getInt(ExecutorStatusChore.WAKE_FREQ, | ||
ExecutorStatusChore.DEFAULT_WAKE_FREQ); | ||
executorStatusChore = new ExecutorStatusChore(sleepTime, this, this.getExecutorService(), |
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.
Are there other executor services running in RS? This the only one?
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.
Not the only one inHRegionServer#startServices()
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.
Hi Stack, maybe I misunderstood it. I mead there is only one ExecutorService, but it will start many Executors.
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.
My question is if this patch will get metrics on all executors in the regionserver or just one set? If it does not get metrics on them all, should it? 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.
So, we only need one chore to report on all executors?
8e37736
to
d6dc72d
Compare
🎊 +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.
This is a nice patch. Just the few little questions about it.
* and report to metrics system | ||
*/ | ||
@InterfaceAudience.Private | ||
public class ExecutorStatusChore extends ScheduledChore { |
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, is it a chore per executorservice monitoried or is it one chore monitoring all executorservices in the process?
HConstants.DEFAULT_EXECUTOR_STATUS_COLLECTE_ENABLED)) { | ||
int sleepTime = this.conf.getInt(ExecutorStatusChore.WAKE_FREQ, | ||
ExecutorStatusChore.DEFAULT_WAKE_FREQ); | ||
executorStatusChore = new ExecutorStatusChore(sleepTime, this, this.getExecutorService(), |
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.
So, we only need one chore to report on all executors?
Hi @saintstack , just one Chore is enough, it will get metrics on all executors in the regionserver. metrics looks like this |
metrics system