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 2020/08/03 21:46:34 UTC
[GitHub] [storm] agresch opened a new pull request #3317: STORM-3682 Upgrade netty client metrics to use V2 API
agresch opened a new pull request #3317:
URL: https://github.com/apache/storm/pull/3317
## 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 a topology LoggingMetricsConsumer enabled and validated metrics appeared.
----------------------------------------------------------------
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 #3317: STORM-3682 Upgrade netty client metrics to use V2 API
Posted by GitBox <gi...@apache.org>.
agresch commented on a change in pull request #3317:
URL: https://github.com/apache/storm/pull/3317#discussion_r467264690
##########
File path: storm-client/src/jvm/org/apache/storm/messaging/netty/Client.java
##########
@@ -420,15 +432,15 @@ public void close() {
private void waitForPendingMessagesToBeSent() {
LOG.info("waiting up to {} ms to send {} pending messages to {}",
- PENDING_MESSAGES_FLUSH_TIMEOUT_MS, pendingMessages.get(), dstAddressPrefixedName);
- long totalPendingMsgs = pendingMessages.get();
+ PENDING_MESSAGES_FLUSH_TIMEOUT_MS, pendingMessages.getCount(), dstAddressPrefixedName);
+ long totalPendingMsgs = pendingMessages.getCount();
long startMs = System.currentTimeMillis();
- while (pendingMessages.get() != 0) {
+ while (pendingMessages.getCount() != 0) {
Review comment:
sounds reasonable. Let me know when you finish the rest of the review and I'll update this.
----------------------------------------------------------------
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 #3317: STORM-3682 Upgrade netty client metrics to use V2 API
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [storm] agresch commented on pull request #3317: STORM-3682 Upgrade netty client metrics to use V2 API
Posted by GitBox <gi...@apache.org>.
agresch commented on pull request #3317:
URL: https://github.com/apache/storm/pull/3317#issuecomment-755738599
closed in preference of 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] agresch closed pull request #3317: STORM-3682 Upgrade netty client metrics to use V2 API
Posted by GitBox <gi...@apache.org>.
agresch closed pull request #3317:
URL: https://github.com/apache/storm/pull/3317
----------------------------------------------------------------
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 #3317: STORM-3682 Upgrade netty client metrics to use V2 API
Posted by 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
[GitHub] [storm] Ethanlm commented on a change in pull request #3317: STORM-3682 Upgrade netty client metrics to use V2 API
Posted by 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. Will it 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
[GitHub] [storm] Ethanlm commented on a change in pull request #3317: STORM-3682 Upgrade netty client metrics to use V2 API
Posted by GitBox <gi...@apache.org>.
Ethanlm commented on a change in pull request #3317:
URL: https://github.com/apache/storm/pull/3317#discussion_r517431135
##########
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 guess it should work. I don't have a better way at this moment.
----------------------------------------------------------------
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 #3317: STORM-3682 Upgrade netty client metrics to use V2 API
Posted by GitBox <gi...@apache.org>.
agresch commented on a change in pull request #3317:
URL: https://github.com/apache/storm/pull/3317#discussion_r468877142
##########
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:
Possibly we could add a new prepare that supports the V2 metrics and keep this prepare (without metric support) and deprecate?
----------------------------------------------------------------
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 #3317: STORM-3682 Upgrade netty client metrics to use V2 API
Posted by GitBox <gi...@apache.org>.
Ethanlm commented on a change in pull request #3317:
URL: https://github.com/apache/storm/pull/3317#discussion_r467153995
##########
File path: storm-client/src/jvm/org/apache/storm/messaging/netty/Client.java
##########
@@ -420,15 +432,15 @@ public void close() {
private void waitForPendingMessagesToBeSent() {
LOG.info("waiting up to {} ms to send {} pending messages to {}",
- PENDING_MESSAGES_FLUSH_TIMEOUT_MS, pendingMessages.get(), dstAddressPrefixedName);
- long totalPendingMsgs = pendingMessages.get();
+ PENDING_MESSAGES_FLUSH_TIMEOUT_MS, pendingMessages.getCount(), dstAddressPrefixedName);
+ long totalPendingMsgs = pendingMessages.getCount();
long startMs = System.currentTimeMillis();
- while (pendingMessages.get() != 0) {
+ while (pendingMessages.getCount() != 0) {
Review comment:
I am worried about performance since meter.getCount() doesn't seem trivial. Maybe we should keep those AtomIntegers and add meters separately. They are essentially different things
##########
File path: docs/Metrics.md
##########
@@ -273,7 +273,12 @@ Be aware that the `__system` bolt is an actual bolt so regular bolt metrics desc
##### Send (Netty Client)
Review comment:
> The value is a map where ...
We need to update the doc there too since the structure changed.
----------------------------------------------------------------
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