Ethanlm commented on a change in pull request #3317:
URL: https://github.com/apache/storm/pull/3317#discussion_r467223578
##########
File path: storm-client/src/jvm/org/apache/storm/messaging/IContext.java
##########
@@ -29,8 +30,11 @@
* This method is invoked at the startup of messaging plugin.
*
* @param topoConf storm configuration
+ * @param metricRegistry metric registry
+ * @param topologyId topology Id
+ * @param workerPort worker port
*/
- void prepare(Map<String, Object> topoConf);
+ void prepare(Map<String, Object> topoConf, StormMetricRegistry
metricRegistry, String topologyId, int workerPort);
Review comment:
I am worried about this interface change since topology can potentially
provide their own IContext implementation.
Secondly, if the context is initialized this way:
`Method method = klass.getMethod("makeContext", Map.class);`
https://github.com/apache/storm/blob/master/storm-client/src/jvm/org/apache/storm/messaging/TransportFactory.java#L42-L45
it will have issues
(I am currently not sure if there is a better way to not change the
interface)
##########
File path: storm-client/src/jvm/org/apache/storm/messaging/netty/Context.java
##########
@@ -67,7 +75,8 @@ public synchronized IConnection bind(String stormId, int
port, IConnectionCallba
@Override
public IConnection connect(String stormId, String host, int port,
AtomicBoolean[] remoteBpStatus) {
return new Client(topoConf, remoteBpStatus, workerEventLoopGroup,
Review comment:
There could be name collisions.
Every time where there is a new connection needs to be established, a new
`Client` is created and the metrics will be registered with the name for
example `send-connection-reconnects-remoteHost1-remotePort1`.
Imaging the remote worker is then rescheduled to another host, say,
host2-port2, then the new metric will be created as
`send-connection-reconnects-remoteHost2-remotePort2` while the old one
`send-connection-reconnects-remoteHost1-remotePort1` still remain in the
metricRegistry.
Then rescheduling happens again, that remote worker is rescheduled back to
host1-port1, a new Client is being created and it tries to register
`send-connection-reconnects-remoteHost1-remotePort1` metric. This is when name
collision happens.
##########
File path: storm-client/src/jvm/org/apache/storm/messaging/netty/Client.java
##########
@@ -163,6 +166,15 @@
waitStrategy = ReflectionUtils.newInstance(clazz);
}
waitStrategy.prepare(topoConf, WaitSituation.BACK_PRESSURE_WAIT);
+
+ totalConnectionAttempts =
stormMetricRegistry.meter("send-connection-reconnects-" + host + ":" + port,
topologyId,
Review comment:
In other places in the code base, we prefix with `__` to indicate it is
an internal metric. So I think we should probably keep following that and keep
using `__send-iconnection` prefix here.
##########
File path: storm-client/src/jvm/org/apache/storm/messaging/netty/Client.java
##########
@@ -89,23 +91,23 @@
/**
* Total number of connection attempts.
*/
- private final AtomicInteger totalConnectionAttempts = new AtomicInteger(0);
+ private final Meter totalConnectionAttempts;
Review comment:
I think these metrics can be gauges so that it simply return and reset
these AtomicIntegers like `totalConnectionAttempts`.
Meter has high performance overhead and it doesn't seem very necessary to
use meter here.
----------------------------------------------------------------
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:
[email protected]
