[GitHub] [storm] Ethanlm commented on a change in pull request #3317: STORM-3682 Upgrade netty client metrics to use V2 API

[GitBox <gi...@apache.org>]

Ethanlm commented on a change in pull request #3317:
URL: https://github.com/apache/storm/pull/3317#discussion_r468129169



##########
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. It will cause `IllegalArgumentException`
   
   
    




----------------------------------------------------------------
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