You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by d2r <gi...@git.apache.org> on 2016/08/10 15:26:15 UTC
[GitHub] storm pull request #1618: [STORM-2032] removes warning in case more than one...
GitHub user d2r opened a pull request:
https://github.com/apache/storm/pull/1618
[STORM-2032] removes warning in case more than one metrics tuple is received
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/d2r/storm storm-2032-remove-not-fast-enough-log
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/1618.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1618
----
commit 45499c4c1259c668a7b03cebf7010f4766610cf3
Author: Derek Dagit <de...@yahoo-inc.com>
Date: 2016-08-10T15:24:04Z
removes warning in case more than one metrics tuple is received
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1618: [STORM-2032] removes warning in case more than one...
Posted by satishd <gi...@git.apache.org>.
Github user satishd commented on a diff in the pull request:
https://github.com/apache/storm/pull/1618#discussion_r74365279
--- Diff: storm-core/src/jvm/org/apache/storm/messaging/netty/StormClientHandler.java ---
@@ -60,7 +60,6 @@ public void messageReceived(ChannelHandlerContext ctx, MessageEvent event) {
//This should be the metrics, and there should only be one of them
List<TaskMessage> list = (List<TaskMessage>)message;
if (list.size() < 1) throw new RuntimeException("Didn't see enough load metrics ("+client.getDstAddress()+") "+list);
- if (list.size() != 1) LOG.warn("Messages are not being delivered fast enough, got "+list.size()+" metrics messages at once("+client.getDstAddress()+")");
--- End diff --
nit: Can we have a debug statement saying more than one task message received instead of the removed warning message?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1618: [STORM-2032] removes warning in case more than one metric...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1618
+1 Great finding.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1618: [STORM-2032] removes warning in case more than one...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1618#discussion_r74367640
--- Diff: storm-core/src/jvm/org/apache/storm/messaging/netty/StormClientHandler.java ---
@@ -60,7 +60,6 @@ public void messageReceived(ChannelHandlerContext ctx, MessageEvent event) {
//This should be the metrics, and there should only be one of them
List<TaskMessage> list = (List<TaskMessage>)message;
if (list.size() < 1) throw new RuntimeException("Didn't see enough load metrics ("+client.getDstAddress()+") "+list);
- if (list.size() != 1) LOG.warn("Messages are not being delivered fast enough, got "+list.size()+" metrics messages at once("+client.getDstAddress()+")");
--- End diff --
I'm also OK for this approach. If we pick this, comment doesn't need to be modified.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1618: [STORM-2032] removes warning in case more than one metric...
Posted by satishd <gi...@git.apache.org>.
Github user satishd commented on the issue:
https://github.com/apache/storm/pull/1618
+1, would like minor nit to be addressed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1618: [STORM-2032] removes warning in case more than one...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1618#discussion_r74361223
--- Diff: storm-core/src/jvm/org/apache/storm/messaging/netty/StormClientHandler.java ---
@@ -60,7 +60,6 @@ public void messageReceived(ChannelHandlerContext ctx, MessageEvent event) {
//This should be the metrics, and there should only be one of them
--- End diff --
If we want to remove warning message because this is not a part of design, how about modifying this comment as well?
Maybe like this : `This should be the metrics, and it always picks the last one when more than one metrics messages are available.`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1618: [STORM-2032] removes warning in case more than one metric...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on the issue:
https://github.com/apache/storm/pull/1618
OK, no problem. Changed it to debug.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1618: [STORM-2032] removes warning in case more than one...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/1618
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---