You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by GitBox <gi...@apache.org> on 2021/01/06 21:58:04 UTC
[GitHub] [storm] agresch opened a new pull request #3371: STORM-3682 Upgrade netty client metrics to use V2 API
agresch opened a new pull request #3371:
URL: https://github.com/apache/storm/pull/3371
## What is the purpose of the change
This change switches the Netty Client send connection metrics to the V2 metrics API.
## How was the change tested
Ran storm-client/storm-core unit tests. Ran topology and validated metrics were logged. Killed some workers and validated clients on others properly deregistered metrics for the killed client.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [storm] Ethanlm commented on a change in pull request #3371: STORM-3682 Upgrade netty client metrics to use V2 API
Posted by GitBox <gi...@apache.org>.
Ethanlm commented on a change in pull request #3371:
URL: https://github.com/apache/storm/pull/3371#discussion_r560330472
##########
File path: docs/Metrics.md
##########
@@ -268,25 +268,11 @@ Be aware that the `__system` bolt is an actual bolt so regular bolt metrics desc
##### Send (Netty Client)
-The `__send-iconnection` metric holds information about all of the clients for this worker. It is of the form
+The `__send-iconnection` metrics report information about all of the clients for this worker. They are named __send-connection-METRIC_TYPE-HOST:PORT for a given Client that is
Review comment:
nit: change ` __send-connection` to `__send-iconnection-`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [storm] agresch merged pull request #3371: STORM-3682 Upgrade netty client metrics to use V2 API
Posted by GitBox <gi...@apache.org>.
agresch merged pull request #3371:
URL: https://github.com/apache/storm/pull/3371
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [storm] Ethanlm commented on a change in pull request #3371: STORM-3682 Upgrade netty client metrics to use V2 API
Posted by GitBox <gi...@apache.org>.
Ethanlm commented on a change in pull request #3371:
URL: https://github.com/apache/storm/pull/3371#discussion_r556924977
##########
File path: storm-client/src/jvm/org/apache/storm/metrics2/TaskMetricRepo.java
##########
@@ -34,23 +34,33 @@
private SortedMap<String, Timer> timers = new TreeMap<>();
public void addCounter(String name, Counter counter) {
- counters.put(name, counter);
+ synchronized (this) {
+ counters.put(name, counter);
+ }
}
public void addGauge(String name, Gauge gauge) {
- gauges.put(name, gauge);
+ synchronized (this) {
+ gauges.put(name, gauge);
+ }
}
public void addMeter(String name, Meter meter) {
- meters.put(name, meter);
+ synchronized (this) {
Review comment:
Maybe you explained this before. But why do they need to be `SortedMap`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [storm] bipinprasad commented on a change in pull request #3371: STORM-3682 Upgrade netty client metrics to use V2 API
Posted by GitBox <gi...@apache.org>.
bipinprasad commented on a change in pull request #3371:
URL: https://github.com/apache/storm/pull/3371#discussion_r556167382
##########
File path: storm-client/src/jvm/org/apache/storm/metrics2/TaskMetricRepo.java
##########
@@ -34,23 +34,33 @@
private SortedMap<String, Timer> timers = new TreeMap<>();
public void addCounter(String name, Counter counter) {
- counters.put(name, counter);
+ synchronized (this) {
Review comment:
Looks like all the synchronization blocks are the fully body of the function, i.e. equivalent synchronized methods. If the intent is to make it finer grained, then the lock should be on "counters" in the add....() methods and for each treeMap in the deregisterMetrics().
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [storm] Ethanlm commented on a change in pull request #3371: STORM-3682 Upgrade netty client metrics to use V2 API
Posted by GitBox <gi...@apache.org>.
Ethanlm commented on a change in pull request #3371:
URL: https://github.com/apache/storm/pull/3371#discussion_r556932098
##########
File path: storm-client/src/jvm/org/apache/storm/metrics2/TaskMetricRepo.java
##########
@@ -34,23 +34,33 @@
private SortedMap<String, Timer> timers = new TreeMap<>();
public void addCounter(String name, Counter counter) {
- counters.put(name, counter);
+ synchronized (this) {
+ counters.put(name, counter);
+ }
}
public void addGauge(String name, Gauge gauge) {
- gauges.put(name, gauge);
+ synchronized (this) {
+ gauges.put(name, gauge);
+ }
}
public void addMeter(String name, Meter meter) {
- meters.put(name, meter);
+ synchronized (this) {
Review comment:
In the reporter method, a list of new SortedMap is created if filter is not null. Maybe we can create a dummy filter that returns true and use it when the filter parameter is null. In this way, `report` method will always report sortedMap.
And we use ConcurrentHashMap outside to reduce performance impact from synchronization.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [storm] agresch commented on a change in pull request #3371: STORM-3682 Upgrade netty client metrics to use V2 API
Posted by GitBox <gi...@apache.org>.
agresch commented on a change in pull request #3371:
URL: https://github.com/apache/storm/pull/3371#discussion_r552990222
##########
File path: storm-client/src/jvm/org/apache/storm/metrics2/StormMetricRegistry.java
##########
@@ -84,13 +86,21 @@ public RateCounter rateCounter(String metricName, String topologyId,
return gauge;
}
+ @Deprecated
public <T> Gauge<T> gauge(String name, Gauge<T> gauge, String topologyId, String componentId, Integer taskId, Integer port) {
Review comment:
the topologyId and port are now available to the registry. We can remove them from all the methods here. I can file a JIRA to do this as a follow on.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [storm] Ethanlm commented on a change in pull request #3371: STORM-3682 Upgrade netty client metrics to use V2 API
Posted by GitBox <gi...@apache.org>.
Ethanlm commented on a change in pull request #3371:
URL: https://github.com/apache/storm/pull/3371#discussion_r556924977
##########
File path: storm-client/src/jvm/org/apache/storm/metrics2/TaskMetricRepo.java
##########
@@ -34,23 +34,33 @@
private SortedMap<String, Timer> timers = new TreeMap<>();
public void addCounter(String name, Counter counter) {
- counters.put(name, counter);
+ synchronized (this) {
+ counters.put(name, counter);
+ }
}
public void addGauge(String name, Gauge gauge) {
- gauges.put(name, gauge);
+ synchronized (this) {
+ gauges.put(name, gauge);
+ }
}
public void addMeter(String name, Meter meter) {
- meters.put(name, meter);
+ synchronized (this) {
Review comment:
Maybe you explained this before. But why do they need to be `SortedMap`? Can we use ConcurrentHashMap?
Do we need synchronization in the `report` method
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [storm] agresch commented on a change in pull request #3371: STORM-3682 Upgrade netty client metrics to use V2 API
Posted by GitBox <gi...@apache.org>.
agresch commented on a change in pull request #3371:
URL: https://github.com/apache/storm/pull/3371#discussion_r558554102
##########
File path: storm-client/src/jvm/org/apache/storm/metrics2/StormMetricRegistry.java
##########
@@ -84,13 +86,21 @@ public RateCounter rateCounter(String metricName, String topologyId,
return gauge;
}
+ @Deprecated
public <T> Gauge<T> gauge(String name, Gauge<T> gauge, String topologyId, String componentId, Integer taskId, Integer port) {
Review comment:
https://issues.apache.org/jira/browse/STORM-3736
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [storm] Ethanlm commented on a change in pull request #3371: STORM-3682 Upgrade netty client metrics to use V2 API
Posted by GitBox <gi...@apache.org>.
Ethanlm commented on a change in pull request #3371:
URL: https://github.com/apache/storm/pull/3371#discussion_r556889361
##########
File path: storm-client/src/jvm/org/apache/storm/messaging/TransportFactory.java
##########
@@ -37,7 +38,7 @@ public static IContext makeContext(Map<String, Object> topoConf) {
//case 1: plugin is a IContext class
transport = (IContext) obj;
//initialize with storm configuration
- transport.prepare(topoConf);
+ transport.prepare(topoConf, metricRegistry);
} else {
//case 2: Non-IContext plugin must have a makeContext(topoConf) method that returns IContext object
Method method = klass.getMethod("makeContext", Map.class);
Review comment:
I am fine with simply ignoring the metricRegistry when the IContext is created this way, and it can be added by invoking a different `makeContext` method that includes `StormMetricRegistry` as a parameter in the future if desired.
Can we add some comments like "StormMetricRegistry is ignored if IContext is created this way" here so it might help future developers? Thanks
##########
File path: storm-client/src/jvm/org/apache/storm/metrics2/TaskMetricRepo.java
##########
@@ -91,4 +101,14 @@ public void report(ScheduledReporter reporter, MetricFilter filter) {
reporter.report(gauges, counters, histograms, meters, timers);
}
}
+
+ void degisterMetrics(MetricFilter metricFilter) {
Review comment:
nit: Should this be `deregister`?
##########
File path: storm-client/src/jvm/org/apache/storm/messaging/netty/Client.java
##########
@@ -163,6 +170,50 @@
waitStrategy = ReflectionUtils.newInstance(clazz);
}
waitStrategy.prepare(topoConf, WaitSituation.BACK_PRESSURE_WAIT);
+ this.metricRegistry = metricRegistry;
+
+ // it's possible to be passed a null metric registry if users are using their own IContext implementation.
+ if (this.metricRegistry != null) {
+ Gauge<Integer> reconnects = new Gauge<Integer>() {
+ @Override
+ public Integer getValue() {
+ return totalConnectionAttempts.get();
+ }
+ };
+ metricRegistry.gauge("__send-connection-reconnects-" + host + ":" + port, reconnects,
Review comment:
We are using different names here and in the doc docs/Metrics.md
`__send-iconnection` vs `__send-connection`
I would prefer to preserve the original name since we have been using it in previous storm versions. But at least let's change one of them and make it consistent.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [storm] Ethanlm commented on a change in pull request #3371: STORM-3682 Upgrade netty client metrics to use V2 API
Posted by GitBox <gi...@apache.org>.
Ethanlm commented on a change in pull request #3371:
URL: https://github.com/apache/storm/pull/3371#discussion_r556924977
##########
File path: storm-client/src/jvm/org/apache/storm/metrics2/TaskMetricRepo.java
##########
@@ -34,23 +34,33 @@
private SortedMap<String, Timer> timers = new TreeMap<>();
public void addCounter(String name, Counter counter) {
- counters.put(name, counter);
+ synchronized (this) {
+ counters.put(name, counter);
+ }
}
public void addGauge(String name, Gauge gauge) {
- gauges.put(name, gauge);
+ synchronized (this) {
+ gauges.put(name, gauge);
+ }
}
public void addMeter(String name, Meter meter) {
- meters.put(name, meter);
+ synchronized (this) {
Review comment:
Maybe you explained this before. But why do they need to be `SortedMap`? Can we use ConcurrentHashMap?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [storm] bipinprasad commented on a change in pull request #3371: STORM-3682 Upgrade netty client metrics to use V2 API
Posted by GitBox <gi...@apache.org>.
bipinprasad commented on a change in pull request #3371:
URL: https://github.com/apache/storm/pull/3371#discussion_r556168871
##########
File path: storm-client/src/jvm/org/apache/storm/metrics2/StormMetricRegistry.java
##########
@@ -432,4 +450,35 @@ public void run() {
}
}
}
+
+ private static class RemoveMetricFilter implements MetricFilter {
+ private Set<Metric> metrics = new HashSet<>();
+
+ RemoveMetricFilter(Set<Metric> toRemove) {
+ this.metrics.addAll(toRemove);
+ for (Metric metric : toRemove) {
+ // RateCounters are gauges, but also have internal Counters that should also be removed
+ if (metric instanceof RateCounter) {
+ RateCounter rateCounter = (RateCounter) metric;
+ this.metrics.add(rateCounter.getCounter());
+ }
+ }
+ }
+
+ /**
+ * Returns {@code true} if the metric matches the filter; {@code false} otherwise.
+ *
+ * @param name the metric's name
+ * @param metric the metric
+ * @return {@code true} if the metric matches the filter
+ */
+ @Override
+ public boolean matches(String name, Metric metric) {
+ if (this.metrics.contains(metric)) {
Review comment:
nit: superfluous conditional: replace with return this.metrics.contains(metric)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [storm] agresch commented on a change in pull request #3371: STORM-3682 Upgrade netty client metrics to use V2 API
Posted by GitBox <gi...@apache.org>.
agresch commented on a change in pull request #3371:
URL: https://github.com/apache/storm/pull/3371#discussion_r556928859
##########
File path: storm-client/src/jvm/org/apache/storm/metrics2/TaskMetricRepo.java
##########
@@ -34,23 +34,33 @@
private SortedMap<String, Timer> timers = new TreeMap<>();
public void addCounter(String name, Counter counter) {
- counters.put(name, counter);
+ synchronized (this) {
+ counters.put(name, counter);
+ }
}
public void addGauge(String name, Gauge gauge) {
- gauges.put(name, gauge);
+ synchronized (this) {
+ gauges.put(name, gauge);
+ }
}
public void addMeter(String name, Meter meter) {
- meters.put(name, meter);
+ synchronized (this) {
Review comment:
I believe the reporters require a sorted map. If we don't use one, we'd have to create one every report call. We should add synchronization here too.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org