You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by zentol <gi...@git.apache.org> on 2017/10/11 13:16:40 UTC
[GitHub] flink pull request #4803: [FLINK-7813] [metrics] Replace MetricRegistryThrea...
GitHub user zentol opened a pull request:
https://github.com/apache/flink/pull/4803
[FLINK-7813] [metrics] Replace MetricRegistryThreadFactory
## What is the purpose of the change
This PR modifies the MetricRegistry to use the ExecutorThreadFactory instead of its own MetricRegistrThreadFactory. The implementations are virtually identical, so this primarily serves
code-deduplication.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/zentol/flink 7813
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/4803.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #4803
----
commit 3bdebd26a25012410c702994a0f92716cbca0d93
Author: zentol <ch...@apache.org>
Date: 2017-10-11T13:13:00Z
[FLINK-7813] [metrics] Replace MetricRegistryThreadFactory
----
---
[GitHub] flink pull request #4803: [FLINK-7813] [metrics] Replace MetricRegistryThrea...
Posted by zentol <gi...@git.apache.org>.
Github user zentol closed the pull request at:
https://github.com/apache/flink/pull/4803
---
[GitHub] flink issue #4803: [FLINK-7813] [metrics] Replace MetricRegistryThreadFactor...
Posted by GJL <gi...@git.apache.org>.
Github user GJL commented on the issue:
https://github.com/apache/flink/pull/4803
👍
I reviewed the code and can confirm that the implementations are virtually identical.
---
[GitHub] flink issue #4803: [FLINK-7813] [metrics] Replace MetricRegistryThreadFactor...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/4803
merging.
---
[GitHub] flink pull request #4803: [FLINK-7813] [metrics] Replace MetricRegistryThrea...
Posted by GJL <gi...@git.apache.org>.
Github user GJL commented on a diff in the pull request:
https://github.com/apache/flink/pull/4803#discussion_r145122089
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/metrics/MetricRegistry.java ---
@@ -402,25 +401,4 @@ public void run() {
}
}
}
-
- private static final class MetricRegistryThreadFactory implements ThreadFactory {
- private final ThreadGroup group;
- private final AtomicInteger threadNumber = new AtomicInteger(1);
-
- MetricRegistryThreadFactory() {
- SecurityManager s = System.getSecurityManager();
- group = (s != null) ? s.getThreadGroup() : Thread.currentThread().getThreadGroup();
- }
-
- public Thread newThread(Runnable r) {
- Thread t = new Thread(group, r, "Flink-MetricRegistry-" + threadNumber.getAndIncrement(), 0);
--- End diff --
The thread name will be different after this change. New thread naming scheme is `<poolName>-thread-<threadNumber>`.
---
[GitHub] flink pull request #4803: [FLINK-7813] [metrics] Replace MetricRegistryThrea...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/4803#discussion_r145347968
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/metrics/MetricRegistry.java ---
@@ -402,25 +401,4 @@ public void run() {
}
}
}
-
- private static final class MetricRegistryThreadFactory implements ThreadFactory {
- private final ThreadGroup group;
- private final AtomicInteger threadNumber = new AtomicInteger(1);
-
- MetricRegistryThreadFactory() {
- SecurityManager s = System.getSecurityManager();
- group = (s != null) ? s.getThreadGroup() : Thread.currentThread().getThreadGroup();
- }
-
- public Thread newThread(Runnable r) {
- Thread t = new Thread(group, r, "Flink-MetricRegistry-" + threadNumber.getAndIncrement(), 0);
--- End diff --
That's true. It's not important imo; in fact I would argue that it's actually an argument for merging it to make the names more consistent.
---