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:
us...@infra.apache.org
