-
Notifications
You must be signed in to change notification settings - Fork 909
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
fix lost prometheus metric in OrderedExecutor #4374
fix lost prometheus metric in OrderedExecutor #4374
Conversation
@merlimat Could you take a look of this problem? |
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.
Nice catch.
I think we can change createSingleThreadExecutor
return SingleThreadExecutor directly.
BTW, I prefer split this pr, one to fix problem, one to modify the metric name.
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.
LGTM,
I think this kind of metric change require adding a testcase to cover it
ok, I would split to 2 pr. But I guess we can't change |
6814b3c
to
aa3ee80
Compare
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.
Would you mind update the description since we split the PR? I would be glad to proceed with the merge once the updates are made.
Thanks for your contribution. :) |
Fix [#4373](#4373) ### Motivation As shown in the issue. This is the first pr to fix the lost metric problem. And the other pr improve the metric in SingleThreadExecutor. Co-authored-by: fanjianye <[email protected]> (cherry picked from commit e5300a0)
Fix [#4373](#4373) ### Motivation As shown in the issue. This is the first pr to fix the lost metric problem. And the other pr improve the metric in SingleThreadExecutor. Co-authored-by: fanjianye <[email protected]> (cherry picked from commit e5300a0)
@@ -391,6 +391,10 @@ protected OrderedExecutor(String baseName, int numThreads, ThreadFactory threadF | |||
ExecutorService thread = createSingleThreadExecutor( | |||
new ThreadFactoryBuilder().setNameFormat(name + "-" + getClass().getSimpleName() + "-" + i + "-%d") | |||
.setThreadFactory(threadFactory).build()); | |||
SingleThreadExecutor ste = null; | |||
if (thread instanceof SingleThreadExecutor) { |
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 can registerMetrics here instead of the instanceOf 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.
Do you mean that remove the check if (thread instanceof SingleThreadExecutor)
directly ?
But createSingleThreadExecutor() is also triggered in OrderedScheduler. In OrderedScheduler, createSingleThreadExecutor() do not return SingleThreadExecutor, so I keep the check here.
Fix [apache#4373](apache#4373) ### Motivation As shown in the issue. This is the first pr to fix the lost metric problem. And the other pr improve the metric in SingleThreadExecutor. Co-authored-by: fanjianye <[email protected]>
Descriptions of the changes in this PR:
Fix #4373
Motivation
As shown in the issue.
This is the first pr to fix the lost metric problem. And the other pr improve the metric in SingleThreadExecutor.
Changes
If we only fix the problem but do not change the metric , the metric is as follow, the metric become visible, though the format is not same.