-
Notifications
You must be signed in to change notification settings - Fork 566
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
[4.x] - Remove non-virtual executor support #7324
Conversation
Signed-off-by: Dmitry Aleksandrov <dmitry.aleksandrov@oracle.com>
21a2b83
to
3e771c3
Compare
Signed-off-by: Dmitry Aleksandrov <dmitry.aleksandrov@oracle.com>
After some investigations I found out that
@tomas-langer and @tjquinno, please share your thoughts. |
Signed-off-by: Dmitry Aleksandrov <dmitry.aleksandrov@oracle.com>
Generally, I have mixed feelings about the value of keeping this feature. The argument can be made that, in 4.x, exposing this metric is less important than in earlier releases with Helidon's use of virtual threads. TBH, I do not have very strong feelings either way about keeping the metric (and therefore having to continue using reflection which we like to avoid) vs. removing the metric. If there are effective actions an operations person can take after monitoring this metric, then that does argue for keeping it. If not, then not! |
if (executorService instanceof ThreadPoolExecutor) { | ||
registerMetrics((ThreadPoolExecutor) executorService, index); | ||
} |
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.
If we are going to keep this feature (and the addition of the type checking before casting is good), then we might as well use instanceof
pattern matching:
if (executorService instance ThreadPoolExecutor tpe) {
registerMetrics(tpe, index);
}
I know it's a small stylistic point and functionally no different from doing explicit casting (ThreadPoolExecutor)
, but it is more concise and less prone to typos.
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.
Applied! Thank you!
Signed-off-by: Dmitry Aleksandrov <dmitry.aleksandrov@oracle.com>
IF we want to keep this feature I'm OK with the changes in the PR. The question is whether we want to keep the metric. As I said, I could be convinced either way. |
@tjquinno please decide yourself, then either approve or request changes ;) |
Resolves #7272
Removes non-virtual executor support