You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by BigOneLiu <gi...@git.apache.org> on 2017/10/17 13:05:48 UTC
[GitHub] storm pull request #2377: [STORM-2780] MetricsConsumer record unnecessary ti...
GitHub user BigOneLiu opened a pull request:
https://github.com/apache/storm/pull/2377
[STORM-2780] MetricsConsumer record unnecessary timestamp
@revans2 Could you please review my pr? thx
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/BigOneLiu/storm master1017
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/2377.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 #2377
----
commit f3f182db9ef194161de6e42edcefe1d378604146
Author: liuxin <li...@zte.com.cn>
Date: 2017-10-17T12:57:55Z
[STORM-2780] MetricsConsumer record unnecessary timestamp
----
---
[GitHub] storm issue #2377: [STORM-2780] MetricsConsumer record unnecessary timestamp
Posted by BigOneLiu <gi...@git.apache.org>.
Github user BigOneLiu commented on the issue:
https://github.com/apache/storm/pull/2377
@vesense
Could you please review my pr?
If it could be an improvement?
---
[GitHub] storm issue #2377: [STORM-2780] MetricsConsumer record unnecessary timestamp
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2377
@BigOneLiu When the task info is created
https://github.com/apache/storm/blob/124acb92dff04a57b530ab4d95a698abc8ff46d9/storm-client/src/jvm/org/apache/storm/executor/Executor.java#L291-L293
The time that is set there is when the metric was sent (in seconds). In most cases it should not take much if any time for it to make its way to the metrics consumer, but if it does the times could be off from one another.
I personally am +1 for this change, because I don't really use this time when I am looking at logged metrics, but if others want it they should speak up.
---
[GitHub] storm issue #2377: [STORM-2780] MetricsConsumer record unnecessary timestamp
Posted by vesense <gi...@git.apache.org>.
Github user vesense commented on the issue:
https://github.com/apache/storm/pull/2377
I prefer to Ieave it as it is, since the log time and the taskInfo.timestamp is not same. Perhaps someone uses it, I'm not sure.
---
[GitHub] storm issue #2377: [STORM-2780] MetricsConsumer record unnecessary timestamp
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2377
@BigOneLiu actually like I sad they are **not** coming from the same location. 99.9% of the time they are probably going to be the same underlying value, but if for some reason the metrics tuple is delayed by 1 second they are 100% guaranteed to be different.
@vesense if you are concerned about this that is probably a good enough reason to not do it. Users are going to have enough on their hands going to 2.x where we may even be moving to an entirely new metrics system.
---
[GitHub] storm issue #2377: [STORM-2780] MetricsConsumer record unnecessary timestamp
Posted by BigOneLiu <gi...@git.apache.org>.
Github user BigOneLiu commented on the issue:
https://github.com/apache/storm/pull/2377
Actually you can easily get the taskInfo.timestamp(1508207360) from thr date (2017-10-17 10:29:20) ,and Log4j framework has provided more accurate date(2017-10-17 10:29:20,906 126908).
If users who don't know MetricsConsumer read this log file, they may be confused.
So I think it's better to delete it.
---
[GitHub] storm issue #2377: [STORM-2780] MetricsConsumer record unnecessary timestamp
Posted by BigOneLiu <gi...@git.apache.org>.
Github user BigOneLiu commented on the issue:
https://github.com/apache/storm/pull/2377
@revans2 like you said 99.9% of the time they are the same,and if the tuple is delayed by 1 second,it's a big problem for storm.maybe you don't need this data then.
vesense's consideration is good enough but I don't mean to delete the attribute in taskInfo.I just delete it from log file in case user get confused.
users can call taskInfo.timestamp whenever they want to use it in new metrics system.
I just concern about this,if you think it's not a good step I would no longer insist.
---
[GitHub] storm pull request #2377: [STORM-2780] MetricsConsumer record unnecessary ti...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/2377
---