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 2016/07/27 09:41:08 UTC
[GitHub] flink pull request #2300: [FLINK-4245] Expose all possible variables
GitHub user zentol opened a pull request:
https://github.com/apache/flink/pull/2300
[FLINK-4245] Expose all possible variables
This PR allows users and reporters to access a `Map<String, String>` of all defined variables and their associated values via `MetricGroup#getAllVariables()`.
The following changes were made:
* Every `AbstractMetricGroup` now has a parent field (previously, each `ComponentMetricGroup` had it's own field). For proper typing, both of these now have a generic type argument, denoting the type of the parent.
* For example: `TaskMetricGroup extends ComponentMetricGroup<TaskManagerJobMetricGroup>`
* most super() calls had to be adjusted to pass the parent as well
* The method `Map<String, String> getAllVariables()` was added to the `MetricGroup` interface
* Non-`ComponentMetricGroup` implementations always forward the call to the parent or return an empty map.
* A `ComponentMetricGroup` creates a new `HashMap` (if it wasn't created before), enters it's own variables (see next item), and adds the values returned by `parent.getAllVariables()`
* The method `protected void putVariables(Map<String, String> variables)` was added to the `ComponentMetricGroup` class.
* In this method the group adds it's varaibles and values into the map
* The map is lazily computed.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/zentol/flink metrics_tags
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/2300.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 #2300
----
commit a0d7b8bc7a518f981da91570f5b20f2d09a095eb
Author: zentol <ch...@apache.org>
Date: 2016-07-27T09:25:27Z
[FLINK-4245] Expose all possible variables
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2300: [FLINK-4245] [metrics] Expose all defined variable...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/2300#discussion_r75112856
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/metrics/groups/AbstractMetricGroup.java ---
@@ -54,13 +53,19 @@
* return Counters, Gauges, etc to the code, to prevent exceptions in the monitored code.
* These metrics simply do not get reported any more, when created on a closed group.
*/
-public abstract class AbstractMetricGroup implements MetricGroup {
+public abstract class AbstractMetricGroup<A extends AbstractMetricGroup> implements MetricGroup {
--- End diff --
Curious, what is the reason to introduce a generically typed parent?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2300: [FLINK-4245] [metrics] Expose all defined variable...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/2300#discussion_r75111320
--- Diff: flink-metrics/flink-metrics-core/src/main/java/org/apache/flink/metrics/groups/UnregisteredMetricsGroup.java ---
@@ -87,6 +90,11 @@ public MetricGroup addGroup(String name) {
}
@Override
+ public Map<String, String> getAllVariables() {
+ return new HashMap<>();
--- End diff --
For empty sets `Collections.emptyMap()` is most efficient.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2300: [FLINK-4245] [metrics] Expose all defined variable...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2300#discussion_r75117479
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/metrics/groups/JobManagerMetricGroup.java ---
@@ -95,6 +96,15 @@ public int numRegisteredJobMetricGroups() {
return jobs.size();
}
+ // ------------------------------------------------------------------------
+ // Component Metric Group Specifics
+ // ------------------------------------------------------------------------
+
+ @Override
+ protected void putVariables(Map<String, String> variables) {
+ variables.put(ScopeFormat.SCOPE_ACTOR_HOST, hostname);
--- End diff --
Agreed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2300: [FLINK-4245] [metrics] Expose all defined variables
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/2300
I am fine with either solution to the double-check locking issue: making the variable volatile, or eagerly creating the map.
Pick the one you prefer, and then this should be good to merge.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2300: [FLINK-4245] [metrics] Expose all defined variables
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/2300
I've rebased the PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2300: [FLINK-4245] [metrics] Expose all defined variable...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/2300#discussion_r75112762
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/metrics/groups/JobManagerMetricGroup.java ---
@@ -95,6 +96,15 @@ public int numRegisteredJobMetricGroups() {
return jobs.size();
}
+ // ------------------------------------------------------------------------
+ // Component Metric Group Specifics
+ // ------------------------------------------------------------------------
+
+ @Override
+ protected void putVariables(Map<String, String> variables) {
+ variables.put(ScopeFormat.SCOPE_ACTOR_HOST, hostname);
--- End diff --
Can we call this simply `SCOPE_HOST`? The "actor" part is an implementation specific detail that should not be part of this abstraction level.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2300: [FLINK-4245] [metrics] Expose all defined variables
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/2300
We can easily expose the tags in the JMXReporter as shown here: https://github.com/zentol/flink/commit/395bb82eccd78c8b065a10ec129abc91cc6ffc05
Essentially, i added a new method to all JMX beans that return the tags map.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2300: [FLINK-4245] [metrics] Expose all defined variable...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/2300#discussion_r75112407
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/metrics/groups/AbstractMetricGroup.java ---
@@ -83,9 +88,25 @@
// ------------------------------------------------------------------------
- public AbstractMetricGroup(MetricRegistry registry, String[] scope) {
+ public AbstractMetricGroup(MetricRegistry registry, String[] scope, A parent) {
this.registry = checkNotNull(registry);
this.scopeComponents = checkNotNull(scope);
+ this.parent = parent;
+ }
+
+ public Map<String, String> getAllVariables() {
+ if (variables == null) { // avoid synchronization for common case
--- End diff --
This "double check locking" does not work properly, unless you make the variable `variables` volatile.
I am wondering if we should not simply initialize the map eagerly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2300: [FLINK-4245] [metrics] Expose all defined variable...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/2300#discussion_r75113495
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/metrics/groups/AbstractMetricGroup.java ---
@@ -54,13 +53,19 @@
* return Counters, Gauges, etc to the code, to prevent exceptions in the monitored code.
* These metrics simply do not get reported any more, when created on a closed group.
*/
-public abstract class AbstractMetricGroup implements MetricGroup {
+public abstract class AbstractMetricGroup<A extends AbstractMetricGroup> implements MetricGroup {
--- End diff --
We'll probably get generic warnings here. Would be good to properly declare it as `AbstractMetricGroup<A extends AbstractMetricGroup<?>>`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2300: [FLINK-4245] [metrics] Expose all defined variable...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2300#discussion_r75115976
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/metrics/groups/AbstractMetricGroup.java ---
@@ -54,13 +53,19 @@
* return Counters, Gauges, etc to the code, to prevent exceptions in the monitored code.
* These metrics simply do not get reported any more, when created on a closed group.
*/
-public abstract class AbstractMetricGroup implements MetricGroup {
+public abstract class AbstractMetricGroup<A extends AbstractMetricGroup> implements MetricGroup {
--- End diff --
I think it is only to prevent us from having a separate parent field in every group implementation.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2300: [FLINK-4245] [metrics] Expose all defined variable...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2300#discussion_r75119021
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/metrics/groups/AbstractMetricGroup.java ---
@@ -83,9 +88,25 @@
// ------------------------------------------------------------------------
- public AbstractMetricGroup(MetricRegistry registry, String[] scope) {
+ public AbstractMetricGroup(MetricRegistry registry, String[] scope, A parent) {
this.registry = checkNotNull(registry);
this.scopeComponents = checkNotNull(scope);
+ this.parent = parent;
+ }
+
+ public Map<String, String> getAllVariables() {
+ if (variables == null) { // avoid synchronization for common case
--- End diff --
well this is interesting. Making the access more expensive could pose problems for the JMX exposure I linked, as it calls `getAllVariables()` for every metric. Would the locking issue still apply if we initialize the maps eagerly, but insert the values lazily? Instead of a null-check we would use `Map#isEmpty()`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2300: [FLINK-4245] [metrics] Expose all defined variable...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2300#discussion_r75190837
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/metrics/groups/AbstractMetricGroup.java ---
@@ -83,9 +88,25 @@
// ------------------------------------------------------------------------
- public AbstractMetricGroup(MetricRegistry registry, String[] scope) {
+ public AbstractMetricGroup(MetricRegistry registry, String[] scope, A parent) {
this.registry = checkNotNull(registry);
this.scopeComponents = checkNotNull(scope);
+ this.parent = parent;
+ }
+
+ public Map<String, String> getAllVariables() {
+ if (variables == null) { // avoid synchronization for common case
--- End diff --
Yeah, `isEmpty()` wouldn't work.
I checked my implementation and for JMX it would not be the data-processing thread that calls �getAllVariables()`, so this would in fact be alright. For our current non-JMX reporters this would be done in the respective reporter thread.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2300: [FLINK-4245] [metrics] Expose all defined variables
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/2300
Looks mostly good. I noticed a few things:
I am not quite sure about the lazy initialization. The idea is good, but the current double-check-locking pattern is not multi-thread safe (http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html). Either we make each access a little more expensive (declare the field `volatile`), or we eagerly initialize the maps.
As a followup, we should probably look into adjusting the JMX reporter. Is there a way we can expose all tags?
Can you also rebase this to the latest master? It has a bunch of merge conflicts right now, when trying to test it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2300: [FLINK-4245] [metrics] Expose all defined variables
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/2300
I am taking a look at this now...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2300: [FLINK-4245] [metrics] Expose all defined variable...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/2300
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2300: [FLINK-4245] [metrics] Expose all defined variable...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/2300#discussion_r75152566
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/metrics/groups/AbstractMetricGroup.java ---
@@ -83,9 +88,25 @@
// ------------------------------------------------------------------------
- public AbstractMetricGroup(MetricRegistry registry, String[] scope) {
+ public AbstractMetricGroup(MetricRegistry registry, String[] scope, A parent) {
this.registry = checkNotNull(registry);
this.scopeComponents = checkNotNull(scope);
+ this.parent = parent;
+ }
+
+ public Map<String, String> getAllVariables() {
+ if (variables == null) { // avoid synchronization for common case
--- End diff --
I think this goes into a tricky realm. Judging the consequences of these loose concurrency tricks is quite hard.
In your suggestion, I think that if isEmpty() is false, it may still be being filled, and then other accessors get concurrent modification exceptions.
Who accesses the `volatile` variable? The reporter threads? In that case, the minimal performance hit is not too bad (it would not be in the data processing threads). If the `getAllVariables()` function is actually called on every group anyways for the most common reporter (JMX), then we might just as well build the map eagerly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---